Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Data Source: azurerm_storage_container #5374

Merged

Conversation

minzhang28
Copy link
Contributor

This is to address the request of support storage account container:
#5310

Test Result

=== RUN   TestAccDataSourceArmStorageContainer_basic
=== PAUSE TestAccDataSourceArmStorageContainer_basic
=== CONT  TestAccDataSourceArmStorageContainer_basic
--- PASS: TestAccDataSourceArmStorageContainer_basic (130.38s)
PASS

@minzhang28 minzhang28 force-pushed the added-storage-container-datasource branch from 46c049c to e82256b Compare January 12, 2020 06:58
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @minzhang28

Thanks for this PR :)

I've taken a look through and left some comments inline but this is off to a good start - if we can fix up the comments (and the tests pass) then this otherwise LGTM 👍

Thanks!

azurerm/internal/services/storage/client/client.go Outdated Show resolved Hide resolved
website/docs/d/storage_account_container.html.markdown Outdated Show resolved Hide resolved
website/docs/d/storage_account_container.html.markdown Outdated Show resolved Hide resolved
website/docs/d/storage_account_container.html.markdown Outdated Show resolved Hide resolved
website/docs/d/storage_account_container.html.markdown Outdated Show resolved Hide resolved
@tombuildsstuff tombuildsstuff changed the title added data source for storage container New Data Source: azurerm_storage_container Jan 13, 2020
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @minzhang28

Thanks for pushing those changes - I've taken a look through and run the tests which led to a few more comments but this is looking good to me; if we can fix up the comments (and the tests pass) then this otherwise looks good to me 👍

Thanks!

resource.TestCheckResourceAttr(data.ResourceName, "name", "containerdstest-"+data.RandomString),
resource.TestCheckResourceAttr(data.ResourceName, "container_access_type", "private"),
resource.TestCheckResourceAttr(data.ResourceName, "has_immutability_policy", "false"),
resource.TestCheckResourceAttr(data.ResourceName, "storage_account_name", "acctestsadsc"+data.RandomString),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we need this field to be set in order to look it up, we can remove this:

Suggested change
resource.TestCheckResourceAttr(data.ResourceName, "storage_account_name", "acctestsadsc"+data.RandomString),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this was missed - can we remove this?

website/docs/d/storage_container.html.markdown Outdated Show resolved Hide resolved
@tombuildsstuff tombuildsstuff added this to the v1.42.0 milestone Jan 14, 2020
@minzhang28 minzhang28 force-pushed the added-storage-container-datasource branch from 580e06d to 2f372f7 Compare January 15, 2020 04:48
@minzhang28
Copy link
Contributor Author

hey @minzhang28

Thanks for pushing those changes - I've taken a look through and run the tests which led to a few more comments but this is looking good to me; if we can fix up the comments (and the tests pass) then this otherwise looks good to me 👍

Thanks!

Hi @tombuildsstuff
Thank you for your comments.
I've addressed all of them and test passed on my end, please let me know if there's any changes in your mind needs to be changed, thanks again.

@ghost ghost removed the waiting-response label Jan 15, 2020
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @minzhang28

Thanks for pushing those changes - taking a look through besides one minor comment (which I hope you don't mind but I'm going to push a commit for to get this merged) this otherwise LGTM 👍

Thanks!

resource.TestCheckResourceAttr(data.ResourceName, "name", "containerdstest-"+data.RandomString),
resource.TestCheckResourceAttr(data.ResourceName, "container_access_type", "private"),
resource.TestCheckResourceAttr(data.ResourceName, "has_immutability_policy", "false"),
resource.TestCheckResourceAttr(data.ResourceName, "storage_account_name", "acctestsadsc"+data.RandomString),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this was missed - can we remove this?

this is required to lookup the data source, so if it's omited the test'll fail at another point
@tombuildsstuff tombuildsstuff modified the milestones: v1.42.0, v1.41.0 Jan 15, 2020
@tombuildsstuff
Copy link
Contributor

Tests pass:

Screenshot 2020-01-15 at 09 16 39

@tombuildsstuff tombuildsstuff merged commit 0e8431f into hashicorp:master Jan 15, 2020
tombuildsstuff added a commit that referenced this pull request Jan 15, 2020
@ghost
Copy link

ghost commented Jan 16, 2020

This has been released in version 1.41.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.41.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Mar 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants