-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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 Resource: azurerm_storage_management_policy
#3819
New Resource: azurerm_storage_management_policy
#3819
Conversation
df7cbec
to
5f6115a
Compare
e57e013
to
7b7460a
Compare
2d65183
to
5f6115a
Compare
Looks like the issue has been fixed and released :) |
Any update on this? |
8fe3ea8
to
1539a50
Compare
6d6600b
to
cd75159
Compare
cd75159
to
63d7099
Compare
I've just rebased these changes to resolve a conflict in |
Any updates on this? Thanks. |
@batizar I'm waiting on review to see if there are any other changes required for merging. When I spoke with @tombuildsstuff he was expecting to look at this next week |
Thanks @stuartleeks |
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.
Hey @stuartleeks these changes look good but there are some areas where we could make improvements. The bulk of changes to make are just nil checks. Any time we are casting from an interface{}
we should confirm that it's not nil. If it is nil and we try to cast it, Terraform crashes and then people could lose state so we need to limit it wherever we can. I called it out in a few places but just comb through and make sure that we're doing those checks everywhere. The other things I pointed out are just best practices we should consider adding to this PR.
1c1555f
to
d538c85
Compare
Co-Authored-By: Matthew Frahry <mbfrahry@gmail.com>
4539bc5
to
81ac702
Compare
81ac702
to
7c541b2
Compare
@mbfrahry - thanks for your thorough review. I've been through your review comments and believe that I have now addressed them. I've also rebased to handle conflicts with other merged changes. Let me know if there are any further changes you would like to see before merging. |
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.
Awesome! Thanks for going through and addressing all my concerns @stuartleeks. These all look good and we're good to merge.
azurerm_storage_management_policy
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
Add support for Storage Management Policies to archive/delete blobs
Fixes #3316