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.

Sign up for GitHub

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

Jump to bottom

azurerm_storage_account - add supports for change_feed_retention_in_days #17130

Merged
merged 1 commit into from
Jul 20, 2022
Merged

azurerm_storage_account - add supports for change_feed_retention_in_days #17130

merged 1 commit into from
Jul 20, 2022

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Jun 7, 2022

Fix #17117

Test

💢  TF_ACC=1 go test -v -timeout=20h ./internal/services/storage -run='TestAccStorageAccount_blobProperties$'
=== RUN   TestAccStorageAccount_blobProperties
=== PAUSE TestAccStorageAccount_blobProperties
=== CONT  TestAccStorageAccount_blobProperties
--- PASS: TestAccStorageAccount_blobProperties (485.50s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/storage       485.513s

waszak
waszak suggested changes Jun 20, 2022
View reviewed changes
Copy link

@waszak waszak left a comment

Choose a reason for hiding this comment

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

I think your implementation should be consistent with the rest of the code.
Check how delete_retention_policy, container_delete_retention_policy are implemented.

internal/services/storage/storage_account_resource.go
@@ -403,6 +403,12 @@ func resourceStorageAccount() *pluginsdk.Resource {
Default: false,
},

"change_feed_retention_in_days": {
Copy link

Choose a reason for hiding this comment

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

It should be more like other retention_policies
example

	"delete_retention_policy": {
							Type:     pluginsdk.TypeList,
							Optional: true,
							MaxItems: 1,
							Elem: &pluginsdk.Resource{
								Schema: map[string]*pluginsdk.Schema{
									"days": {
										Type:         pluginsdk.TypeInt,
										Optional:     true,
										Default:      7,
										ValidateFunc: validation.IntBetween(1, 365),
									},
								},
							},
						},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. But the problem is that there already exists a property change_feed_enabled. If it doesn't, we can use the same style as other retention policies, and use the presence of this block to indicate whether this policy is enabled or not.

Besides, the allowed maximum day is 14600 days (i.e. 400 years) as is stated in the swagger.

Copy link

Choose a reason for hiding this comment

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

No its 365 and default is 7
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bug in Portal, the API can use a larger value. Also the default value used in TF is not necessarily needed to be the same as Portal.

internal/services/storage/storage_account_resource.go
@@ -2815,7 +2830,8 @@ func flattenBlobProperties(input storage.BlobServiceProperties) []interface{} {
"cors_rule": flattenedCorsRules,
"delete_retention_policy": flattenedDeletePolicy,
"versioning_enabled": versioning,
"change_feed_enabled": changeFeed,
"change_feed_enabled": changeFeedEnabled,
"change_feed_retention_in_days": changeFeedRetentionInDays,
Copy link

Choose a reason for hiding this comment

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

change_feed_retention_policy

website/docs/r/storage_account.html.markdown
@@ -172,6 +172,8 @@ A `blob_properties` block supports the following:

* `change_feed_enabled` - (Optional) Is the blob service properties for change feed events enabled? Default to `false`.

* `change_feed_retention_in_days` - (Optional) The duration of change feed events retention in days. The possible values are between 1 and 146000 days (400 years). Setting this to null (or omit this in the configuration file) indicates an infinite retention of the change feed.
Copy link

Choose a reason for hiding this comment

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

A change_feed_retention_policy block supports the following:

  • days - (Optional) Specifies the number of days that the change feed should be retained, between 1 and 365 days. Defaults to 7.

@katbyte
Copy link
Collaborator

katbyte commented Jul 11, 2022

have a test failure:
image

@magodo
Copy link
Collaborator Author

magodo commented Jul 11, 2022 •
edited
Loading

@katbyte This test failure is also observed from the main: https://ci-oss.hashicorp.engineering/buildConfiguration/TerraformOpenSource_TerraformProviders_AzureRMPublic_AZURERM_SERVICE_PUBLIC_STORAGE/310986, which seems intermittently happen, and might be a service side issue.

@katbyte
Copy link
Collaborator

katbyte commented Jul 11, 2022

@magodo - can you open an issue with the service team and drop a link to it here for tracking?

@magodo
Copy link
Collaborator Author

magodo commented Jul 15, 2022

@katbyte The issue is opened: Azure/azure-rest-api-specs#19785

katbyte
katbyte approved these changes Jul 20, 2022
View reviewed changes
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 @magodo - LGTM 🌮

@katbyte katbyte merged commit bc8508b into hashicorp:main Jul 20, 2022
katbyte added a commit that referenced this pull request Jul 20, 2022
@katbyte
CHANGELOG #17130
961cefa
@github-actions github-actions bot added this to the v3.15.0 milestone Jul 20, 2022
@github-actions
Copy link

This functionality has been released in v3.15.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@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 Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@waszak waszak waszak requested changes

@katbyte katbyte katbyte approved these changes

Assignees
No one assigned
Labels
documentation enhancement service/storage size/S
Projects
None yet
Milestone
v3.15.0
Development

Successfully merging this pull request may close these issues.

Support for change_feed_retention_in_days on azurerm_storage_account
3 participants
@magodo @katbyte @waszak