-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 NotificationConfig to google_container_cluster #4012
Add NotificationConfig to google_container_cluster #4012
Conversation
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. @ndmckinley, please review this PR or find an appropriate assignee. |
Ping @megan07 - this is the PR to replace hashicorp/terraform-provider-google#7338. I can't seem to add you as a reviewer. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 225 insertions(+), 2 deletions(-)) |
/gcbrun |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148414" |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 225 insertions(+), 2 deletions(-)) |
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.
Hi @freddd! This looks great! Just a few comments on the tests and I think we'll be ready to merge!
Thank you!
templates/terraform/examples/container_cluster_notification_config.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_container_cluster_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_container_cluster_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_container_cluster_test.go.erb
Outdated
Show resolved
Hide resolved
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 202 insertions(+), 2 deletions(-)) |
/gcbrun |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=148612" |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 202 insertions(+), 2 deletions(-)) |
I can't seem to see why it failed. Cloud build is stating |
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.
Hi @freddd! Sorry for the confusion, I run our entire test suite against previously recorded responses from the API to check that no other tests have been affected - and recently that run has been timing out and showing a failure, but I double-check them and will let you know if there are any errors to worry about. Thanks again!
notification_config { | ||
pubsub { | ||
enabled = true | ||
topic = "google_pubsub_topic.%s.id" |
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.
Sorry, I missed this! This shouldn't be in quotes, we want to extrapolate the value from google_pubsub_topic.%s.id
, we don't want that string here. Then it should be good!
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.
No worries! I removed the quotes in 3862c14
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 202 insertions(+), 2 deletions(-)) |
/gcbrun |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=149151" |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 202 insertions(+), 2 deletions(-)) |
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.
Looks great! Tests pass! Thank you!!
This PR aims to introduce the new beta attribute NotificationConfig to google_container_cluster. This is the issue I created in the terraform provider (though in the wrong provider, I should've added it to the beta provider): hashicorp/terraform-provider-google#7337
In order to test this PR I ran:
bundle exec compiler -a -v "beta" -e terraform -o "$GOPATH/src/github.com/hashicorp/terraform-provider-google-beta"
to generate the providermake test
in the providermake lint
in the providerrake test
in magic modulesReferences:
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)