Skip to content

Conversation

@stevenyang72
Copy link
Member

@stevenyang72 stevenyang72 commented May 27, 2025

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.


Remove ID validation on WorkloadIdentityPool Namespace and Managed Identity, clean up associated test files.

Context: #14048 (comment)

@github-actions github-actions bot requested a review from melinath May 27, 2025 16:53
@github-actions
Copy link

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.

@melinath, 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
Copy link
Collaborator

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 ( 1 file changed, 2 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 6 files changed, 10 insertions(+), 155 deletions(-))
terraform-google-conversion: Diff ( 2 files changed, 96 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 37
Passed tests: 31
Skipped tests: 2
Affected tests: 4

Click here to see the affected service packages
  • iambeta
#### Action taken
Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccIAMBetaWorkloadIdentityPoolNamespace_full
  • TestAccIAMBetaWorkloadIdentityPoolNamespace_iamWorkloadIdentityPoolNamespaceBasicExample
  • TestAccIAMBetaWorkloadIdentityPoolNamespace_iamWorkloadIdentityPoolNamespaceFullExample
  • TestAccIAMBetaWorkloadIdentityPoolNamespace_minimal

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccIAMBetaWorkloadIdentityPoolNamespace_full [Debug log]
TestAccIAMBetaWorkloadIdentityPoolNamespace_iamWorkloadIdentityPoolNamespaceBasicExample [Debug log]
TestAccIAMBetaWorkloadIdentityPoolNamespace_iamWorkloadIdentityPoolNamespaceFullExample [Debug log]
TestAccIAMBetaWorkloadIdentityPoolNamespace_minimal [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

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

Validation functions are generally useful because they allow errors to happen at plan time instead of apply time. We do sometimes remove them if there's evidence that the validation won't be stable over time - is that the case here? Otherwise I don't think there's a need to remove this validation.

Is there some additional context for this change? Like a bug that the validation is causing?

cc @zli82016 as the commenter of #14048 (comment)

@stevenyang72
Copy link
Member Author

stevenyang72 commented May 27, 2025

Validation functions are generally useful because they allow errors to happen at plan time instead of apply time. We do sometimes remove them if there's evidence that the validation won't be stable over time - is that the case here? Otherwise I don't think there's a need to remove this validation.

Is there some additional context for this change? Like a bug that the validation is causing?

cc @zli82016 as the commenter of #14048 (comment)

Managed Workload Identity (MWLID) Namespace and Managed Identity are parts of a set of new API resources provided for the managed workload identities feature. I'm not sure if there'll be changes on the server-side validation, but IMO the change would probably rarely happen because that will likely be a breaking change.

I'm afraid this is all the context I have for this one. Might need @zli82016's input here.

@github-actions github-actions bot requested a review from melinath May 27, 2025 23:18
@zli82016
Copy link
Member

Validation functions are generally useful because they allow errors to happen at plan time instead of apply time. We do sometimes remove them if there's evidence that the validation won't be stable over time - is that the case here? Otherwise I don't think there's a need to remove this validation.

Is there some additional context for this change? Like a bug that the validation is causing?

cc @zli82016 as the commenter of #14048 (comment)

There isn't a bug related to the validation itself. My main concern is simply that I remember we prefer to avoid client-side validation, as it could cause issue if the validation changes on the service side.

@melinath
Copy link
Member

Ack, thanks @zli82016!

@stevenyang72 is this field likely to have looser validation in the future? If so we should just remove the validation; if it's expected to be static we should probably leave the validation in place.

@melinath
Copy link
Member

oh you already said:

I'm not sure if there'll be changes on the server-side validation, but IMO the change would probably rarely happen because that will likely be a breaking change.

I think I'd lean towards leaving the validation there in that case.

@melinath melinath closed this May 28, 2025
@stevenyang72
Copy link
Member Author

stevenyang72 commented May 28, 2025

oh you already said:

I'm not sure if there'll be changes on the server-side validation, but IMO the change would probably rarely happen because that will likely be a breaking change.

I think I'd lean towards leaving the validation there in that case.

Got it. Thanks! Shall I open another PR to bring back the ID validation on Managed Identity then? @melinath

@melinath
Copy link
Member

@stevenyang72 I checked with other folks on the team just to be sure and it sounds like we generally allow validation of ID segments like this. If you'd like to open a PR to add that validation back you're welcome to.

@stevenyang72
Copy link
Member Author

Hi @melinath, thanks for the confirmation! Opened #14126 to bring back the validation on workload_identity_pool_managed_identity_id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants