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

azurerm_storage_accountsupport for change_feed, versioning_enabled, default_service_version , and last_access_time_enabled within the blob_properties` block #11301

Merged
merged 9 commits into from
Apr 25, 2021

Conversation

yupwei68
Copy link
Contributor

@yupwei68 yupwei68 commented Apr 13, 2021

Extends #9277
Fix:
#8268
#7722
#6627

=== RUN TestAccStorageAccount_blobProperties
=== PAUSE TestAccStorageAccount_blobProperties
=== CONT TestAccStorageAccount_blobProperties
--- PASS: TestAccStorageAccount_blobProperties (274.03s)

Since restore_policy has some update problem, it'll be enhanced when the fix is released.

retentionDays in ChangeFeed is only supported in canary regions now, all regions will be supported soon.

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.

hi @yupwei68

Thanks for this PR.

I've taken a look through here and left some comments inline, if we can fix those up then we should be able to take another look - but in particular if we can fix up the descriptions to be clearer that'd be great.

Thanks!

Comment on lines +15 to +38
"2008-10-27",
"2009-04-14",
"2009-07-17",
"2009-09-19",
"2011-08-28",
"2012-02-12",
"2013-08-15",
"2014-02-14",
"2015-02-21",
"2015-04-05",
"2015-07-08",
"2015-12-11",
"2016-05-31",
"2017-04-17",
"2017-07-29",
"2017-11-09",
"2018-03-28",
"2018-11-09",
"2019-02-02",
"2019-07-07",
"2019-12-12",
"2020-02-10",
"2020-04-08",
"2020-06-12",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "2008-10-27" and "2020-06-12" are supported, but not list in the list. I've tested these two.

website/docs/r/storage_account.html.markdown Outdated Show resolved Hide resolved
website/docs/r/storage_account.html.markdown Outdated Show resolved Hide resolved
Comment on lines 305 to 324
"name": {
Type: schema.TypeString,
Optional: true,
Default: "AccessTimeTracking",
ValidateFunc: validation.StringInSlice([]string{"AccessTimeTracking"}, false),
},
"granularity_in_days": {
Type: schema.TypeInt,
Optional: true,
Default: 1,
ValidateFunc: validation.IntInSlice([]int{1}),
},
"blob_type": {
Type: schema.TypeList,
Optional: true,
Computed: true,
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validation.StringInSlice([]string{"blockBlob"}, false),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

a few things here:

  1. granularity_in_days and blob_type can't be set, so should be computed-only
  2. there's a single possible value for name and blob_type - so these likely shouldn't be exposed

as such this block could likely be removed in place of a single field for last_access_time_tracking_policy_enabled - unless there's plans by the service team to add other functionality here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The service plans to support the granularity_in_days and blob_type in the future. I just thought that it may be hard to covert last_access_time_tracking_policy from bool to list in the future.
Currently azurerm_management_policy is dependent on the last_access_time_tracking_policy.
But I will change it into bool now.

"retention_in_days": {
Type: schema.TypeInt,
Optional: true,
ValidateFunc: validation.IntBetween(1, 146000),
Copy link
Contributor

Choose a reason for hiding this comment

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

146000/365 = 400

This is 400 years worth of changes.. is there a realistic maximum value from the service team here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It's 400 years.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minimum value is 1 day and maximum value is 146000 days (400 years).

@yupwei68
Copy link
Contributor Author

Hi Tom, thanks for your comments. Changes have been pushed. Tests have all passed. Please continue reviewing.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @yupwei68 - aside from one comment on a property name this looks good to me

@yupwei68
Copy link
Contributor Author

Hi kt, thanks for your comments. Tests have passed. The CI has failed for linter broken. Please continue reviewing.
=== RUN TestAccStorageAccount_blobProperties
=== PAUSE TestAccStorageAccount_blobProperties
=== CONT TestAccStorageAccount_blobProperties
--- PASS: TestAccStorageAccount_blobProperties (687.90s)

@yupwei68 yupwei68 requested a review from katbyte April 25, 2021 01:48
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

@yupwei68 - ci needs to be fix but otherwise looks goof

@katbyte katbyte added this to the v2.57.0 milestone Apr 25, 2021
@katbyte katbyte merged commit 5bb934c into hashicorp:master Apr 25, 2021
@katbyte katbyte changed the title azurerm_storage_account blob_properties enhancement azurerm_storage_accountsupport for change_feed, versioning_enabled, default_service_version , and last_access_time_enabled within the blob_properties` block Apr 25, 2021
katbyte added a commit that referenced this pull request Apr 25, 2021
@yupwei68 yupwei68 deleted the wyp-storage-blobproperties2 branch April 26, 2021 06:27
katbyte pushed a commit that referenced this pull request Apr 26, 2021
…nd fix (#11471)

Remove change_feed because retention_in_days in change_feed is blocked to be released now.

Fix when only versioning are enabled.

Fix PR #11301

=== RUN TestAccStorageAccount_blobProperties
=== PAUSE TestAccStorageAccount_blobProperties
=== CONT TestAccStorageAccount_blobProperties
@ghost
Copy link

ghost commented Apr 30, 2021

This has been released in version 2.57.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 = "~> 2.57.0"
}
# ... other configuration ...

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2021
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.

3 participants