-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add azurerm_storage_container_immutability_policy #879
Conversation
Couldn't run the whole make reviewable because it kept crashing my machine but I ran make generate and tested all the changes I did here manually in our tenant. closes #804 |
/test-examples="examples/storage/v1beta1/containerimmutabilitypolicy.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your efforts @Mikel-Landa, left a few comments for you to consider.
config/containerservice/config.go
Outdated
@@ -93,6 +93,9 @@ func Configure(p *config.Provider) { | |||
TerraformName: "azurerm_kubernetes_cluster", | |||
Extractor: rconfig.ExtractResourceIDFuncPath, | |||
} | |||
r.LateInitializer = config.LateInitializer{ | |||
ConditionalIgnoredFields: []string{"node_count"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add an explanation to the PR description about why we ignored this field? Also, can you please test the resource and verify that the issue is gone? It would be great if you could add the test results to the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this change from the PR. Initially, I saw this work but I guess it was because the nodeCount was updated in the MR. After a day or so, when the pool triggered scale up it didn't work again. I would need help with this. It should work if I just put it in initProvider without any more changes to begin with, but neither that or this change actually does anything.
config/externalname.go
Outdated
"azurerm_storage_data_lake_gen2_filesystem": storageDataLakeGen2Filesystem(), | ||
"azurerm_storage_encryption_scope": config.TemplatedStringAsIdentifier("name", "{{ .parameters.storage_account_id }}/encryptionScopes/{{ .external_name }}"), | ||
"azurerm_storage_management_policy": config.TemplatedStringAsIdentifier("", "{{ .parameters.storage_account_id }}/managementPolicies/default"), | ||
"azurerm_storage_container_immutability_policy": config.TemplatedStringAsIdentifier("", "{{ .parameters.storage_container_resource_manager_id }}/immutabilityPolicies/default"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking just to be sure, is the default
string in the .../immutabilityPolicies/default
section fixed, is it the same for every resource created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there can only be one and has to be named default.
Terraform resource import: https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/storage_container_immutability_policy#import
meta.upbound.io/example-id: storage/v1beta1/containerimmutabilitypolicy | ||
labels: | ||
testing.upbound.io/example-name: example | ||
name: examplesa5398912221 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We got the following error, please use a different name.
logger.go:42: 21:44:09 | case/0-apply | async create failed: failed to create the resource: [***0 creating Storage Account (Subscription: "2895a7df-ae9f-41b8-9e78-3ce4926df838"
logger.go:42: 21:44:09 | case/0-apply | Resource Group Name: "example"
logger.go:42: 21:44:09 | case/0-apply | Storage Account Name: "examplesa5398912221"): performing Create: unexpected status 409 (409 Conflict) with error: StorageAccountAlreadyTaken: The storage account named examplesa5398912221 is already taken. []***]
Description of your changes
Fixes #
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested