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 request_reason provider support #5037

Merged
merged 8 commits into from
Aug 12, 2021
Merged

Add request_reason provider support #5037

merged 8 commits into from
Aug 12, 2021

Conversation

salrashid123
Copy link
Contributor

@salrashid123 salrashid123 commented Aug 4, 2021

fixes hashicorp/terraform-provider-google#9724

PR Description

...

provider: Add provider support for `request_reason`

@google-cla google-cla bot added the cla: yes label Aug 4, 2021
@modular-magician
Copy link
Collaborator

Oops! It looks like no changelog entry is attached to this PR. Please include a release note block in the PR body, as described in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md:

```release-note:TYPE
Release note
```

@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.

@rileykarson, 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 ( 4 files changed, 12 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 13 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 2 files changed, 5 insertions(+), 2 deletions(-))

@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 ( 4 files changed, 16 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 17 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 2 files changed, 5 insertions(+), 2 deletions(-))

@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 ( 4 files changed, 16 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 4 files changed, 17 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 2 files changed, 5 insertions(+), 2 deletions(-))

@rileykarson
Copy link
Member

rileykarson commented Aug 5, 2021

Our CI got broken, would you mind rebasing? That should make it run correctly.

mmv1/third_party/terraform/utils/header_transport.go Outdated Show resolved Hide resolved
@@ -148,6 +148,8 @@ the provider should wait for a single HTTP request. This will not adjust the
amount of time the provider will wait for a logical operation - use the resource
timeout blocks for that.

* `request_reason` - (Optional) Send a Request Reason [System Parameter](https://cloud.google.com/apis/docs/system-parameters) for each API call made by the provider. The `X-Goog-Request-Reason` header value is used to provide a user-supplied justification into GCP AuditLogs. The value set within the provider is overridden if `CLOUDSDK_CORE_REQUEST_REASON` environment variable is used.
Copy link
Member

Choose a reason for hiding this comment

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

This should be added under "Full Reference" as well. For consistency with the rest of the file, we should only include the env var there and use text like Alternatively, this can be specified using the 'CLOUDSDK_CORE_REQUEST_REASON' environment variable.

Copy link
Member

Choose a reason for hiding this comment

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

Provider config will override the environment variable w/ this implementation (which is the correct behaviour imo) so the docs should reflect as much!

@melinath
Copy link
Member

melinath commented Aug 5, 2021

/gcbrun may be all that's required (rather than needing to rebase)

@modular-magician

This comment has been minimized.

@melinath

This comment has been minimized.

@modular-magician

This comment has been minimized.

@salrashid123
Copy link
Contributor Author

didn't do the rebase per previous comments but did address comments above

@modular-magician

This comment has been minimized.

@melinath
Copy link
Member

melinath commented Aug 5, 2021

/gcbrun - CI issue is now resolved

@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 ( 4 files changed, 21 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 6 files changed, 25 insertions(+), 8 deletions(-))
TF Conversion: Diff ( 2 files changed, 5 insertions(+), 5 deletions(-))

@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 ( 4 files changed, 21 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 5 files changed, 23 insertions(+), 7 deletions(-))
TF Conversion: Diff ( 2 files changed, 5 insertions(+), 5 deletions(-))

Copy link
Contributor

@ScottSuarez ScottSuarez left a comment

Choose a reason for hiding this comment

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

suggest rewriting for a cleaner implementation. headerTransport is an extension of http.Header

mmv1/third_party/terraform/utils/config.go.erb Outdated Show resolved Hide resolved
@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 ( 4 files changed, 20 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 5 files changed, 22 insertions(+), 7 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 4 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=200384

mmv1/third_party/terraform/utils/provider.go.erb Outdated Show resolved Hide resolved
@@ -148,6 +148,8 @@ the provider should wait for a single HTTP request. This will not adjust the
amount of time the provider will wait for a logical operation - use the resource
timeout blocks for that.

* `request_reason` - (Optional) Send a Request Reason [System Parameter](https://cloud.google.com/apis/docs/system-parameters) for each API call made by the provider. The `X-Goog-Request-Reason` header value is used to provide a user-supplied justification into GCP AuditLogs. The value set within the provider is overridden if `CLOUDSDK_CORE_REQUEST_REASON` environment variable is used.
Copy link
Member

Choose a reason for hiding this comment

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

Provider config will override the environment variable w/ this implementation (which is the correct behaviour imo) so the docs should reflect as much!

@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 ( 4 files changed, 20 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 5 files changed, 21 insertions(+), 5 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 4 deletions(-))

@modular-magician
Copy link
Collaborator

Error trying to cancel build ()

@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 ( 4 files changed, 20 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 5 files changed, 21 insertions(+), 5 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 4 deletions(-))

@modular-magician
Copy link
Collaborator

Error trying to cancel build ()

Copy link
Contributor

@ScottSuarez ScottSuarez left a comment

Choose a reason for hiding this comment

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

seems reasonable to me

@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 ( 4 files changed, 20 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 6 files changed, 23 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 4 deletions(-))

@modular-magician
Copy link
Collaborator

Error trying to cancel build ()

1 similar comment
@modular-magician
Copy link
Collaborator

Error trying to cancel build ()

@ScottSuarez
Copy link
Contributor

/gcbrun

@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 ( 4 files changed, 20 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 5 files changed, 21 insertions(+), 5 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 4 deletions(-))

@modular-magician
Copy link
Collaborator

Error trying to cancel build ()

2 similar comments
@modular-magician
Copy link
Collaborator

Error trying to cancel build ()

@modular-magician
Copy link
Collaborator

Error trying to cancel build ()

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=200705

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccComputeRouterPeer_advertiseMode You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=200804

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

@ScottSuarez: You looked at this more recently (w/ the linked issue below), I'll leave the merge to you

@ScottSuarez
Copy link
Contributor

tested locally, both pathways work

Screen Shot 2021-08-12 at 10 04 47 AM
Screen Shot 2021-08-12 at 10 08 06 AM

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.

Support X-Goog-Request-Reason system parameter
5 participants