Add storage_control_folder_intelligence_config resource. #13394

kautikdk · 2025-03-19T07:28:52Z

This PR adds support for GCS Management Hub on GCP Folder level.

Related Issue(Doesn't fix): hashicorp/terraform-provider-google#20763

Release Note Template for Downstream PRs (will be copied)

`google_storage_control_folder_intelligence_config`

modular-magician · 2025-03-19T07:41:25Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 7 files changed, 1180 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 7 files changed, 1180 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 209 insertions(+))

modular-magician · 2025-03-19T07:44:32Z

Tests analytics

Total tests: 4
Passed tests: 2
Skipped tests: 0
Affected tests: 2

Click here to see the affected service packages

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

Get to know how VCR tests work

modular-magician · 2025-03-19T07:50:25Z

🟢 Tests passed during RECORDING mode:
TestAccDataSourceGoogleStorageControlFolderIntelligenceConfig_basic [Debug log]
TestAccStorageControlFolderIntelligenceConfig_update [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

🟢 All tests passed!

View the build log or the debug log for each test

github-actions · 2025-03-19T09:13:15Z

Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: see go/terraform-auto-test-runs to set up automatic test runs.

@ScottSuarez, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

modular-magician · 2025-03-19T12:08:51Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 9 files changed, 1258 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 9 files changed, 1258 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 209 insertions(+))

modular-magician · 2025-03-19T12:13:40Z

Tests analytics

Total tests: 4
Passed tests: 3
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

Get to know how VCR tests work

modular-magician · 2025-03-19T12:20:02Z

🟢 Tests passed during RECORDING mode:
TestAccStorageControlFolderIntelligenceConfig_update [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

🟢 All tests passed!

View the build log or the debug log for each test

kautikdk · 2025-03-19T12:33:27Z

Hi @trodge, Created this follow up PR to add Storage Intelligence support for Folder hierarchy level. It would be good if you can review this since you have context on the feature and have reviewed previous PR.
Previous PR: #12689

modular-magician · 2025-03-19T12:35:05Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 9 files changed, 1316 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 9 files changed, 1316 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 209 insertions(+))

modular-magician · 2025-03-19T12:37:41Z

Tests analytics

Total tests: 0
Passed tests: 0
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

#### Non-exercised tests

🔴 Tests were added that are skipped in VCR:

TestAccDataSourceGoogleStorageControlFolderIntelligenceConfig_basic
TestAccStorageControlFolderIntelligenceConfig_update
🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.

View the build log

modular-magician · 2025-03-19T13:29:04Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 9 files changed, 1258 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 9 files changed, 1258 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 209 insertions(+))

modular-magician · 2025-03-19T13:34:20Z

Tests analytics

Total tests: 4
Passed tests: 4
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

ScottSuarez · 2025-03-19T20:20:29Z

mmv1/products/storagecontrol/FolderIntelligenceConfig.yaml

+self_link: 'folders/{{name}}/locations/global/intelligenceConfig'
+
+custom_code:
+  custom_create: templates/terraform/custom_create/storage_control_folder_intelligence.go.tmpl


why is custom create needed? pre_create seems sufficient to just alter the url. There is the question of, if we need this to be all on v2 if this is the right way to go about things. Because we will be creating on v2 and reading on v1 with the current implmentation. Open to call to discuss.

Hi @ScottSuarez,
URL modification code was written a quite some time ago before we decided to create a new service storagecontrol for GCS JSON v2. I forgot to remove it, My bad! Regarding custom_create, The reason is the update_mask and PATCH method in create operation. Storage Intelligence can not be created so we mapped create operation to PATCH method but the concern is related to permanent(optional+computed) and optional fields. edition_config is a permanent field and back-filled by server side so user can change but it is not the field that we can clear but the filter block is optional and not back filled by server side. Now just to simulate the create operation, we can put full update_mask of every fields which are optional but no need to send empty value for the fields which are computed or has server side value.
Why we want put update_mask for optional fields?
The main reason is we are clearly stating that the create operation is actually update operation under the hood so the first create operation diff should represent whatever will be the end state and should not generate diff after apply. That necessitates requirement of update_mask for optional fields and if we don't do it then some diff can be generated after apply operation. That will be just similar to import operation that we already support.

I have removed that URL modification line. Open to discuss any concerns here.

Also empty value for edition_config will result in server side error so we can not put update_mask for that field until it is specified by the user in create operation.

+1 to @ScottSuarez's point. This should be possible to do with pre_create custom code rather than replacing the whole create method.

Opened hashicorp/terraform-provider-google#22033 to track making this easier.

I'm a little confused what the concern is with O+C fields; in general, sending an empty value for them (triggering server-side default behavior) should be correct. Although it would be possible to persist the values that already exist on the server side that might be more confusing than beneficial.

Ahh yes, checked how exactly pre-create injects code and realized it adds code snippet just before making an API call. Changed custom_create to pre_create and kept previous update_mask logic as it is while we are still discussing this behavior.

I'm a little confused what the concern is with O+C fields; in general, sending an empty value for them (triggering server-side default behavior) should be correct. Although it would be possible to persist the values that already exist on the server side that might be more confusing than beneficial.

So just to give an example in the Storage Intelligence resource, edition_config is a property of the resource and all the resources are having this field backfilled by the server. Users can configure this field, has server side value initially, and can not be removed so it should be Optional+Computed. Now since we can not remove it, we can not send empty value and update_mask together as it results in a InvalidValue error so we have to put update_mask only when user specifies it otherwise we will keep server side value in the create operation.

So to make sure I understand right, edition_config behaves as follows:

The initial configuration is "empty"

The first time that a user calls this API, they can choose to supply a value; if they don't, the API will choose a value on the server side

Either way, once the field has a value, it can never be changed.

Is that correct?

Umm, Not quite like that.

API behavior is,

The initial value of edition_config field is INHERIT.

User can change the field value but can never remove edition_config property from the resource.

Terraform mapping,

Marked as O+C. That will allow users to keep server side value if not specified in the config and they can change it if they want.

Regarding the empty value reference, I believe my wordings made some confusion. There is an API side validation which checks if the update_mask is provided for edition_config. if it is provided and field is not present in the API request body then it will throw an InvalidValue error.
In the create operation, we will put update_mask of edition_config only when it is specified by user in the resource config. Just like whatever is being done here for every field: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/update_mask.go.tmpl

modular-magician · 2025-03-20T05:05:25Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 9 files changed, 1256 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 9 files changed, 1256 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 209 insertions(+))

modular-magician · 2025-03-20T05:10:28Z

Tests analytics

Total tests: 4
Passed tests: 4
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

kautikdk · 2025-03-20T09:26:42Z

Referring comment from other PR:#13386 (comment)

ScottSuarez

I'm concerned about this resource not having the ability to be cleared. This is a one way set of resource and doesn't appear to be working within the terraform model. Deletion should clear any set config ideally. Given that it does not, I am not sure this api make sense to support at the terraform level.

mmv1/products/storagecontrol/FolderIntelligenceConfig.yaml

kautikdk · 2025-03-21T06:57:59Z

Yes, This resource is different than other GCS resources(in terms of singleton behavior). We have guidelines for singleton resources and there is a mention of resources having no delete endpoint and for them, we have exclude_delete: true which basically skips the deletion rather removes it from the Terraform only. I believe that's the standard Terraform singleton pattern that we are following across TPG though I am don't know about any other similar resources outside of GCS.

Storage Intelligence is a very important feature for large scale GCS resource management and it should be supported through Terraform as well. We have ask from feature team to provide Terraform support with high priority. I will share reference documentations internally.

modular-magician · 2025-03-21T07:34:00Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 9 files changed, 1273 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 9 files changed, 1273 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 209 insertions(+))

modular-magician · 2025-03-21T07:39:14Z

Tests analytics

Total tests: 4
Passed tests: 4
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

modular-magician · 2025-03-21T09:46:39Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 9 files changed, 1287 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 9 files changed, 1287 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 209 insertions(+))

modular-magician · 2025-03-21T09:51:53Z

Tests analytics

Total tests: 4
Passed tests: 4
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

melinath · 2025-03-24T21:05:51Z

@kautikdk is correct that singletons don't need to unset their configuration; this is how they generally work in the provider. Best practices for singleton resources are documented at https://googlecloudplatform.github.io/magic-modules/best-practices/common-resource-patterns/.

modular-magician · 2025-03-25T09:34:08Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 9 files changed, 1287 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 9 files changed, 1287 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 209 insertions(+))

modular-magician · 2025-03-25T09:39:14Z

Tests analytics

Total tests: 6
Passed tests: 6
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

modular-magician · 2025-03-25T10:23:13Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 9 files changed, 1285 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 9 files changed, 1285 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 209 insertions(+))

modular-magician · 2025-03-25T10:28:24Z

Tests analytics

Total tests: 6
Passed tests: 6
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

…Platform#13394)

Adds storage_control_folder_intelligence_config resource.

63605e2

modular-magician added the service/storage label Mar 19, 2025

kautikdk marked this pull request as ready for review March 19, 2025 09:12

github-actions bot requested a review from ScottSuarez March 19, 2025 09:13

kautikdk changed the title ~~Adds storage_control_folder_intelligence_config resource.~~ Add storage_control_folder_intelligence_config resource. Mar 19, 2025

Add empty value tests and fixes formatting issues.

d523aff

Add empty value tests and fixes formatting issues.

fc898fe

Removes duplicate function declarations.

e0b1b64

ScottSuarez requested changes Mar 19, 2025

View reviewed changes

Removes URL modifier

cb82c62

github-actions bot requested a review from ScottSuarez March 20, 2025 04:54

kautikdk mentioned this pull request Mar 20, 2025

Add storage_control_organization_intelligence_config resource. #13386

Merged

ScottSuarez reviewed Mar 20, 2025

View reviewed changes

mmv1/products/storagecontrol/FolderIntelligenceConfig.yaml Show resolved Hide resolved

github-actions bot requested a review from ScottSuarez March 21, 2025 06:58

Adds example documentation.

6da009e

kautikdk force-pushed the folder_storage_intelligence branch from 773c79d to 6da009e Compare March 21, 2025 09:34

Merge branch 'main' into folder_storage_intelligence

4804737

github-actions bot requested a review from melinath March 25, 2025 09:24

Remove custom_create and add pre_create.

41f0f36

ScottSuarez approved these changes Mar 25, 2025

View reviewed changes

ScottSuarez added this pull request to the merge queue Mar 25, 2025

Merged via the queue into GoogleCloudPlatform:main with commit d870a5c Mar 25, 2025
23 of 24 checks passed

This was referenced Mar 26, 2025

Updates SI config resources to utilize pre_create instead of custom_create #13461

Merged

Add documentation for Storage Intelligence data sources. #13463

Merged

This was referenced Mar 26, 2025

Add documentation for Storage Intelligence data sources. hashicorp/terraform-provider-google#22077

Merged

Add documentation for Storage Intelligence data sources. hashicorp/terraform-provider-google-beta#9655

Merged

JaylonmcShan03 pushed a commit to JaylonmcShan03/magic-modules-jmcshan that referenced this pull request Mar 30, 2025

Add storage_control_folder_intelligence_config resource. (GoogleCloud…

12d5d1d

…Platform#13394)

This was referenced Mar 31, 2025

Updates SI config resources to utilize pre_create instead of custom_create hashicorp/terraform-provider-google#22181

Merged

Updates SI config resources to utilize pre_create instead of custom_create hashicorp/terraform-provider-google-beta#9690

Merged

Dawid212 pushed a commit to Dawid212/magic-modules that referenced this pull request Apr 9, 2025

Add storage_control_folder_intelligence_config resource. (GoogleCloud…

256429c

…Platform#13394)

Add storage_control_folder_intelligence_config resource. #13394

Add storage_control_folder_intelligence_config resource. #13394

Conversation

kautikdk commented Mar 19, 2025

modular-magician commented Mar 19, 2025

Diff report

modular-magician commented Mar 19, 2025

Tests analytics

Action taken

modular-magician commented Mar 19, 2025

github-actions bot commented Mar 19, 2025

modular-magician commented Mar 19, 2025

Diff report

modular-magician commented Mar 19, 2025

Tests analytics

Action taken

modular-magician commented Mar 19, 2025

kautikdk commented Mar 19, 2025

modular-magician commented Mar 19, 2025

Diff report

modular-magician commented Mar 19, 2025

Tests analytics

modular-magician commented Mar 19, 2025

Diff report

modular-magician commented Mar 19, 2025

Tests analytics

Choose a reason for hiding this comment

kautikdk Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Mar 20, 2025

Diff report

modular-magician commented Mar 20, 2025

Tests analytics

kautikdk commented Mar 20, 2025

ScottSuarez left a comment

Choose a reason for hiding this comment

kautikdk commented Mar 21, 2025 • edited Loading

modular-magician commented Mar 21, 2025

Diff report

modular-magician commented Mar 21, 2025

Tests analytics

modular-magician commented Mar 21, 2025

Diff report

modular-magician commented Mar 21, 2025

Tests analytics

melinath commented Mar 24, 2025

modular-magician commented Mar 25, 2025

Diff report

modular-magician commented Mar 25, 2025

Tests analytics

modular-magician commented Mar 25, 2025

Diff report

modular-magician commented Mar 25, 2025

Tests analytics

kautikdk Mar 20, 2025 •

edited

Loading

kautikdk commented Mar 21, 2025 •

edited

Loading