-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Reorder credentials precedence so config beats env #5328
Reorder credentials precedence so config beats env #5328
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 107 insertions(+), 62 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccContainerCluster_withAddons|TestAccContainerCluster_withWorkloadIdentityConfigDeprecation|TestAccContainerCluster_withWorkloadIdentityConfig You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=210899 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 107 insertions(+), 62 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccContainerNodePool_withGPU You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=211215 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! thank you so much for submitting this, and for the work you did on docs! I made a few suggestions for style and flow throughout - but please push back if my edits materially change the meaning.
Thank you!
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
Both the `credentials` and `access_token` values can be drawn from the config | ||
directly or specified through environment variables. In earlier versions of the | ||
provider, `access_token` values specified through environment variables took | ||
precedence over `credentials` values specified in config. From `4.0.0` onwards, | ||
config will take precedence over environment variables for those values, and | ||
`access_token` will take precedence over `credentials` if both are specified | ||
through environment variables. | ||
|
||
Note that service account impersonation is unchanged, and will continue to be | ||
used if specified through an environment variable while `credentials` or | ||
`access_token` is specified in config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the `credentials` and `access_token` values can be drawn from the config | |
directly or specified through environment variables. In earlier versions of the | |
provider, `access_token` values specified through environment variables took | |
precedence over `credentials` values specified in config. From `4.0.0` onwards, | |
config will take precedence over environment variables for those values, and | |
`access_token` will take precedence over `credentials` if both are specified | |
through environment variables. | |
Note that service account impersonation is unchanged, and will continue to be | |
used if specified through an environment variable while `credentials` or | |
`access_token` is specified in config. | |
Google can draw values for both the `credentials` and `access_token` from the config | |
directly or from environment variables. | |
In earlier versions of the provider, `access_token` values specified through environment variables took | |
precedence over `credentials` values specified in config. From `4.0.0` onwards, | |
config takes precedence over environment variables, and the `access_token` environment variable takes precedence over the `credential` environment variable. | |
Service account impersonation is unchanged. Google Cloud will continue to use the service account if it is specified through an environment variable, even if `credentials` or | |
`access_token` are specified in config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal as above- Terraform's the one making the call, not GCP, so I'll amend that (but take the rest of the change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Ol' passive vs active voice for the most part, ha. Mentioned a few cases where the meaning is different w/ comments, I'll amend those locally later today and push. Let me know if you'd like me to wait on a second review pass- I'll assume I can move forward if I don't hear anything.
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
Both the `credentials` and `access_token` values can be drawn from the config | ||
directly or specified through environment variables. In earlier versions of the | ||
provider, `access_token` values specified through environment variables took | ||
precedence over `credentials` values specified in config. From `4.0.0` onwards, | ||
config will take precedence over environment variables for those values, and | ||
`access_token` will take precedence over `credentials` if both are specified | ||
through environment variables. | ||
|
||
Note that service account impersonation is unchanged, and will continue to be | ||
used if specified through an environment variable while `credentials` or | ||
`access_token` is specified in config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal as above- Terraform's the one making the call, not GCP, so I'll amend that (but take the rest of the change)
ac14109
to
41278c1
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
Changes to the |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 113 insertions(+), 65 deletions(-)) |
Canceled VCR, it ran on a prior revision & no code changed since. |
Fixes hashicorp/terraform-provider-google#8876
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)