-
Notifications
You must be signed in to change notification settings - Fork 1.8k
make authz extension wire_format O+C #12975
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
make authz extension wire_format O+C #12975
Conversation
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. |
Tests analyticsTotal tests: 57 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_authz_extension" "primary" {
wire_format = # value needed
}
|
Tests analyticsTotal tests: 57 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_authz_extension" "primary" {
wire_format = # value needed
}
|
Tests analyticsTotal tests: 57 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
@modular-magician reassign-reviewer |
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @slevenick, 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. |
@@ -47,21 +47,21 @@ func TestAccNetworkServicesAuthzExtension_update(t *testing.T) { | |||
func testAccNetworkServicesAuthzExtension_start(context map[string]interface{}) string { | |||
return acctest.Nprintf(` | |||
resource "google_compute_network" "default" { | |||
name = "lb-network" | |||
name = "lb-network%{random_suffix}" |
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.
Should we also change these to tf-test
...?
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.
good idea
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_authz_extension" "primary" {
wire_format = # value needed
}
|
Tests analyticsTotal tests: 57 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
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.
Something strange happened with generation, can you repush?
2928502
to
38aae77
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_authz_extension" "primary" {
wire_format = # value needed
}
|
Tests analyticsTotal tests: 57 Click here to see the affected service packages
🟢 All tests passed! View the build log |
fixes hashicorp/terraform-provider-google#21129
fixes hashicorp/terraform-provider-google#21120
fixes hashicorp/terraform-provider-google#21180
fixes hashicorp/terraform-provider-google#21171
not sure if the API changed behavior since resource implementation, but the API no longer accepts the provider supplying the default value. Changing the field to be O+C to allow the API to set the default itself.
Should not be a breaking change due to making it O+C while also moving away from a default that is just straight up invalid.
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.