fix: (storagetransfer) transfer path can be changed to empty string by gurusai-voleti · Pull Request #14300 · GoogleCloudPlatform/magic-modules

gurusai-voleti · 2025-06-18T07:39:20Z

Release Note Template for Downstream PRs (will be copied)
Fixes: hashicorp/terraform-provider-google#13673
See Write release notes for guidance.

storagetransfer: fix to allow empty string for path

modular-magician · 2025-06-18T07:52:34Z

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 ( 3 files changed, 206 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 3 files changed, 206 insertions(+), 7 deletions(-))

modular-magician · 2025-06-18T07:56:16Z

Tests analytics

Total tests: 16
Passed tests: 15
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages

storagetransfer

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

TestAccStorageTransferJob_transferPathError

Get to know how VCR tests work

modular-magician · 2025-06-18T07:56:45Z

🟢 Tests passed during RECORDING mode:
TestAccStorageTransferJob_transferPathError [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

modular-magician · 2025-06-18T08:35:30Z

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 ( 3 files changed, 205 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 3 files changed, 205 insertions(+), 7 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

Field replication_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field replication_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

modular-magician · 2025-06-18T13:19:38Z

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 ( 3 files changed, 205 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 3 files changed, 205 insertions(+), 7 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

Field replication_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field replication_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

modular-magician · 2025-06-18T13:23:22Z

Tests analytics

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

Click here to see the affected service packages

storagetransfer

🟢 All tests passed!

View the build log

github-actions · 2025-06-23T06:46:14Z

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

Googlers: For automatic test runs see go/terraform-auto-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.

kautikdk · 2025-06-24T18:51:25Z

Hi @gurusai-voleti, what is the solution that you are proposing here?

ScottSuarez · 2025-06-24T20:02:55Z

This appears to be a breaking change, see #14300 (comment)

gurusai-voleti · 2025-06-25T05:08:03Z

Hi @gurusai-voleti, what is the solution that you are proposing here?

firstly the field is missing validation, added validation to the path field as gcs object path cannot be started with "/" (reference here https://cloud.google.com/storage-transfer/docs/reference/rest/v1/GcsData) and field is an user provided and cannot be marked as computed so changed path to optional field only

as the customer reported issue they used "/" first and later realized to change to "" empty string
which they cannot as in update d.HasChanges will not detect empty string as a change and an empty string cannot be considered for computed field as per terraform sdk (error occurred with computed set to true A computed value with the empty string as the new value and a non-empty old value was found. Interpreting the empty string as "unset" to align with legacy behavior.) so tf plan/apply will show no changes required, so removed computed for the field and as it is user provided no need of computed as well

as d.HasChanges and computed cannot consider empty string as a change or input, added DiffSuppressFunc if the old and new is same it will not show any difference in tf plan, if its different it will show difference in tf plan and path can be updated to "" empty string

gurusai-voleti · 2025-06-25T05:56:30Z

This appears to be a breaking change, see #14300 (comment)

yes it's a breaking change, so created an PR on top 7.0 feature branch, Anything else needs to be taken care?

kautikdk · 2025-06-25T12:55:37Z

If the field is currently Computed, do we know why it was marked as such? i.e Was there any change from the API side which mandated this field to get server side value as well if not provided by user?

kautikdk · 2025-06-25T09:37:22Z

mmv1/third_party/terraform/services/storagetransfer/resource_storage_transfer_job_test.go

+		Steps: []resource.TestStep{
+			{
+				Config:      testAccStorageTransferJob_transferJobPathError(envvar.GetTestProjectFromEnv(), testDataSourceBucketName, testDataSinkName, testTransferJobDescription, testTransferJobName),
+				ExpectError: regexp.MustCompile(errorMessage),


I don't think we need error validation in the acceptance tests. We should add unit test instead to validate the change which is not related to the API integratiion.

Reference: https://googlecloudplatform.github.io/magic-modules/test/test/#add-resource-tests

removed the validation test case

kautikdk · 2025-06-25T09:38:05Z

mmv1/third_party/terraform/services/storagetransfer/resource_storage_transfer_job_test.go

+  `, project, dataSourceBucketName, project, dataSinkBucketName, project, testTransferJobName, transferJobDescription, project)
+}
+
+func testAccStorageTransferJob_transferJobGcsPath(project string, dataSourceBucketName string, dataSinkBucketName string, transferJobDescription string, testTransferJobName string, gcsPath string) string {


Where is this being used?

updated test case

kautikdk · 2025-06-25T09:39:08Z

mmv1/third_party/terraform/services/storagetransfer/resource_storage_transfer_job.go.tmpl

 				Type:        schema.TypeString,
 				Description: `Google Cloud Storage path in bucket to transfer`,
+				ValidateFunc: validatePath,
+				DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {


If we are removing Computed behavior then do we need this DiffSupressFunc?

May still be relevant if API alters user input

no API doesn't alter user input, removed the diffsuppressfunc

kautikdk · 2025-06-25T13:00:46Z

mmv1/third_party/terraform/website/docs/guides/version_7_upgrade.html.markdown

+## Resource: `google_storage_transfer_job`

-### Resource-level change example header
+### `path` does not allow strings starting with /


We should add full field name here including parent block names otherwise it will mislead users. Only specifying path will be interpreted as top level field within the google_storage_transfer_job resource.

Reference: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/website/docs/guides/version_6_upgrade.html.markdown#advanced_datapath_observability_configrelay_mode-is-now-removed

updated description

gurusai-voleti · 2025-06-25T13:15:21Z

If the field is currently Computed, do we know why it was marked as such? i.e Was there any change from the API side which mandated this field to get server side value as well if not provided by user?

even if its computed or not, as its optional field if API response has value, it can be set otherwise not

modular-magician · 2025-06-25T15:56:27Z

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 ( 3 files changed, 123 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 3 files changed, 123 insertions(+), 7 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

Field replication_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field replication_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

Errors

google-beta provider:

Failed to detect subcategory in the frontmatter in file website/docs/guides/version_7_upgrade.html.markdown.

modular-magician · 2025-06-25T16:02:42Z

Tests analytics

Total tests: 16
Passed tests: 15
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages

storagetransfer

#### 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

TestAccStorageTransferJob_transferUpdateToEmptyString

Get to know how VCR tests work

modular-magician · 2025-06-25T16:03:55Z

🟢 Tests passed during RECORDING mode:
TestAccStorageTransferJob_transferUpdateToEmptyString [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

ScottSuarez · 2025-06-25T16:34:23Z

This appears to be a breaking change, see #14300 (comment)

yes it's a breaking change, so created an PR on top 7.0 feature branch, Anything else needs to be taken care?

oops, I didn't realize we opened for changes to the major release branch. Thanks for that !!

ScottSuarez · 2025-06-25T16:37:19Z

Looks like merging main into this branch has added unintended changes here.

gurusai-voleti · 2025-06-25T16:38:12Z

Looks like merging main into this branch has added unintended changes here.

git rebase went wrong

This reverts commit 04e72e6.

…to sts_path

ScottSuarez · 2025-06-25T17:05:09Z

If the field is currently Computed, do we know why it was marked as such? i.e Was there any change from the API side which mandated this field to get server side value as well if not provided by user?

even if its computed or not, as its optional field if API response has value, it can be set otherwise not

Do we have understanding of when/in what scenario server sets a value for this field independent of user input? While yes we can switch to optional here, I am looking to see how disruptive this might be for the user.

If the user needs to deploy then define this field post deployment because the server returned some value this is not an optimal experience. In such a case we should likely explicitly set the value to empty string if valid to avoid two step apply. As long as such a change isn't very divergent from normal user flow.

modular-magician · 2025-06-25T17:07:27Z

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 ( 3 files changed, 205 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 3 files changed, 205 insertions(+), 7 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

Field replication_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field replication_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

Errors

google-beta provider:

Failed to detect subcategory in the frontmatter in file website/docs/guides/version_7_upgrade.html.markdown.

This reverts commit 00164d2.

modular-magician · 2025-06-25T17:11:14Z

Tests analytics

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

Click here to see the affected service packages

storagetransfer

🟢 All tests passed!

View the build log

modular-magician · 2025-06-25T17:15:46Z

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 ( 3 files changed, 205 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 3 files changed, 205 insertions(+), 7 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

Field replication_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field replication_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

Errors

google-beta provider:

Failed to detect subcategory in the frontmatter in file website/docs/guides/version_7_upgrade.html.markdown.

modular-magician · 2025-06-25T17:19:21Z

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 ( 3 files changed, 123 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 3 files changed, 123 insertions(+), 7 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

Field replication_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field replication_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_sink.path transitioned from optional+computed to optional google_storage_transfer_job - reference
Field transfer_spec.gcs_data_source.path transitioned from optional+computed to optional google_storage_transfer_job - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

Errors

google-beta provider:

Failed to detect subcategory in the frontmatter in file website/docs/guides/version_7_upgrade.html.markdown.

modular-magician · 2025-06-25T17:19:53Z

Tests analytics

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

Click here to see the affected service packages

storagetransfer

🟢 All tests passed!

View the build log

modular-magician · 2025-06-25T17:22:56Z

Tests analytics

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

Click here to see the affected service packages

storagetransfer

🟢 All tests passed!

View the build log

gurusai-voleti · 2025-06-26T07:35:26Z

this branch has been corrupted created a new PR here #14377, please check the new PR @kautikdk @ScottSuarez please add yourself as a reviewer for new PR

gurusai-voleti · 2025-06-26T07:59:02Z

why it was marke

If the field is currently Computed, do we know why it was marked as such? i.e Was there any change from the API side which mandated this field to get server side value as well if not provided by user?

even if its computed or not, as its optional field if API response has value, it can be set otherwise not

Do we have understanding of when/in what scenario server sets a value for this field independent of user input? While yes we can switch to optional here, I am looking to see how disruptive this might be for the user.

If the user needs to deploy then define this field post deployment because the server returned some value this is not an optimal experience. In such a case we should likely explicitly set the value to empty string if valid to avoid two step apply. As long as such a change isn't very divergent from normal user flow.

if the user doesn't provide any value, API doesn't set any value, it just reflects what user provided

fix: path doesnt allow string starting with /

ea1a8a4

modular-magician added the service/storage label Jun 18, 2025

modular-magician requested a review from kautikdk June 18, 2025 07:52

update

09227b5

Update version_7_upgrade.html.markdown

cde54b2

gurusai-voleti marked this pull request as ready for review June 23, 2025 06:45

github-actions bot requested a review from ScottSuarez June 23, 2025 06:46

kautikdk reviewed Jun 25, 2025

View reviewed changes

review comments

04e72e6

modular-magician requested a review from kautikdk June 25, 2025 15:56

Revert "review comments"

00164d2

This reverts commit 04e72e6.

gurusai-voleti force-pushed the sts_path branch from e3b088a to 00164d2 Compare June 25, 2025 16:55

gurusai-voleti added 6 commits June 25, 2025 16:59

fix: path doesnt allow string starting with /

c3f949b

update

76823cf

Update version_7_upgrade.html.markdown

fafefc7

review comments

c52752c

Revert "review comments"

a573a8c

This reverts commit 04e72e6.

Merge branch 'sts_path' of github.com:gurusai-voleti/magic-modules in…

543e005

…to sts_path

Reapply "review comments"

72c8be4

This reverts commit 00164d2.

ScottSuarez closed this Jun 26, 2025

Conversation

gurusai-voleti commented Jun 18, 2025

Uh oh!

modular-magician commented Jun 18, 2025

Diff report

Uh oh!

modular-magician commented Jun 18, 2025

Tests analytics

Action taken

Uh oh!

modular-magician commented Jun 18, 2025

Uh oh!

modular-magician commented Jun 18, 2025

Diff report

Breaking Change(s) Detected

Uh oh!

modular-magician commented Jun 18, 2025

Diff report

Breaking Change(s) Detected

Uh oh!

modular-magician commented Jun 18, 2025

Tests analytics

Uh oh!

github-actions bot commented Jun 23, 2025

Uh oh!

kautikdk commented Jun 24, 2025

Uh oh!

ScottSuarez commented Jun 24, 2025

Uh oh!

gurusai-voleti commented Jun 25, 2025

Uh oh!

gurusai-voleti commented Jun 25, 2025

Uh oh!

kautikdk commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gurusai-voleti commented Jun 25, 2025

Uh oh!

modular-magician commented Jun 25, 2025

Diff report

Breaking Change(s) Detected

Errors

Uh oh!

modular-magician commented Jun 25, 2025

Tests analytics

Uh oh!

modular-magician commented Jun 25, 2025

Uh oh!

ScottSuarez commented Jun 25, 2025

Uh oh!

ScottSuarez commented Jun 25, 2025

Uh oh!

gurusai-voleti commented Jun 25, 2025

Uh oh!

ScottSuarez commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

modular-magician commented Jun 25, 2025

Diff report

Breaking Change(s) Detected

Errors

Uh oh!

modular-magician commented Jun 25, 2025

ScottSuarez commented Jun 25, 2025 •

edited

Loading

gurusai-voleti commented Jun 26, 2025 •

edited

Loading