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

Add warning documenting behavior of the google_service_account_key resource for certain cases where the project cannot be inferred by terraform #5301

Merged
merged 6 commits into from
Feb 16, 2022

Conversation

stangles
Copy link
Contributor

@stangles stangles commented Oct 8, 2021

Resolves hashicorp/terraform-provider-google#9617.

Release Note Template for Downstream PRs (will be copied)

iam: added warning regarding `google_service_account_key` behavior when the project cannot be inferred from the `service_account_id` field.

…source for certain cases where the project cannot be inferred by terraform
@google-cla
Copy link

google-cla bot commented Oct 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Oct 8, 2021
@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@melinath, please review this PR or find an appropriate assignee.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 1 insertion(+))
Terraform Beta: Diff ( 1 file changed, 1 insertion(+))

@stangles
Copy link
Contributor Author

stangles commented Oct 8, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 8, 2021
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.

It seems like it might be better to clarify the docs for service_account_id rather than adding a separate warning.

Also, this warning as written is not entirely correct (and neither is the current service_account_id). The project will be inferred if service_account_id is a full email address OR if it's a full URL like projects/{PROJECT_ID}/serviceAccounts/{ACCOUNT}. It seems like the failure case is if just the first part of the email address is given?

@melinath
Copy link
Member

@stangles are you still working on this PR?

@stangles
Copy link
Contributor Author

It seems like the failure case is if just the first part of the email address is given?

Yes, that matches my understanding. When taking a look back at how I reproduced the unexpected behavior stated in the issue, I believe I had specified the SA unique ID, thus tripping the same error (and also demonstrating how the current service_account_id description isn't correct). Updated in d3ad7a4

are you still working on this PR?

Yes, apologies for the delayed response.

@melinath melinath self-requested a review January 28, 2022 22:08
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.

This looks a lot better but it seems like it should be possible to use projects/{PROJECT_ID}/serviceAccounts/{UNIQUE_ID} according to these docs. Could you incorporate that information as well?

In case it's helpful here's the code the provider uses to calculate the actual service account id sent to the server: https://github.com/hashicorp/terraform-provider-google/blob/master/google/utils.go#L349

@stangles
Copy link
Contributor Author

stangles commented Feb 1, 2022

This looks a lot better but it seems like it should be possible to use projects/{PROJECT_ID}/serviceAccounts/{UNIQUE_ID} according to these docs. Could you incorporate that information as well?

Done in f34e0f1

In case it's helpful here's the code the provider uses

That was quite helpful, thank you for the documentation and code references. Hopefully I've got it right this time around 😄

@melinath melinath self-requested a review February 7, 2022 23:10
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.

This looks a lot better - the one thing that seems to be missing is that I think it should be possible to use projects/{PROJECT_ID}/serviceAccounts/{UNIQUE_ID} - could you include that as an option?

@stangles
Copy link
Contributor Author

I think it should be possible to use projects/{PROJECT_ID}/serviceAccounts/{UNIQUE_ID} - could you include that as an option?

Yep, added in 35423ff which hopefully is unambiguous without being too wordy.

@melinath melinath self-requested a review February 15, 2022 23:18
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.

LGTM - thanks!

@melinath
Copy link
Member

Build failure is unrelated; this is docs only & will be fine after merge.

lcaggio pushed a commit to lcaggio/magic-modules that referenced this pull request Mar 16, 2022
…source for certain cases where the project cannot be inferred by terraform (GoogleCloudPlatform#5301)

* Add warning documenting behavior of the google_service_account_key resource for certain cases where the project cannot be inferred by terraform

* remove warning in favor of rewording service_account_id description

* restore newline

* clarify description to include '-' wildcard substitution

* rewrite to include unique id

* remove sa name as account option from fully qualified syntax
lcaggio pushed a commit to lcaggio/magic-modules that referenced this pull request Mar 17, 2022
…source for certain cases where the project cannot be inferred by terraform (GoogleCloudPlatform#5301)

* Add warning documenting behavior of the google_service_account_key resource for certain cases where the project cannot be inferred by terraform

* remove warning in favor of rewording service_account_id description

* restore newline

* clarify description to include '-' wildcard substitution

* rewrite to include unique id

* remove sa name as account option from fully qualified syntax
betsy-lichtenberg pushed a commit to betsy-lichtenberg/magic-modules that referenced this pull request Apr 25, 2022
…source for certain cases where the project cannot be inferred by terraform (GoogleCloudPlatform#5301)

* Add warning documenting behavior of the google_service_account_key resource for certain cases where the project cannot be inferred by terraform

* remove warning in favor of rewording service_account_id description

* restore newline

* clarify description to include '-' wildcard substitution

* rewrite to include unique id

* remove sa name as account option from fully qualified syntax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

google_service_account_key project: required field is not set
3 participants