-
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
Adding Apigee NAT Address Resource #5018
Adding Apigee NAT Address Resource #5018
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. @rileykarson, please review this PR or find an appropriate assignee. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 537 insertions(+), 4 deletions(-)) |
Sorry, lost email on my end. @melinath do you mind picking up this review, since you've looked at Apigee more extensively? |
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.
This seems reasonable. I'll start a /gcbrun.
org_id = google_apigee_organization.apigee_org.id | ||
} | ||
|
||
resource "google_apigee_nat_address" "apigee_nat_addres" { |
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.
typo: should be apigee_nat_adress
/gcbrun |
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.
It looks like this is failing due to issues in the ansible provider. If you merge master (or rebase your PR) that should address the issue. Could you clarify the issues you had running the acceptance tests for this change?
Sorry that this fell through the cracks. Please feel free to re-request review any time that you're ready for another review.
f33d8e8
to
a3e0dd1
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 537 insertions(+), 4 deletions(-)) |
thanks @melinath rebased the branch and it looks like we need a /gcbrun again for the remaining check. Thanks |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 537 insertions(+), 4 deletions(-)) |
Tests were added that did not run in TeamCity:
|
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccTags You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=208280 |
Doing a specific CI-run for the new test: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/208683 |
Hm. That failed with the following error:
Here's the full request/response:
Is this probably transient, or more likely flakey? (Starting another CI test run for just this test: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/208699) |
(FWIW it looks like that last run failed with the same error.) |
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.
I'm removing this from my review queue pending a response/fix re: the above error.
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 8 files changed, 555 insertions(+), 22 deletions(-)) |
@melinath Could you please re-run the cloud build again? I trust the Apigee instance provisioning has stabilized since we last tried. Thanks |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 537 insertions(+), 4 deletions(-)) |
Tests were added that did not run in TeamCity:
|
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudFunctionsFunction_vpcConnector|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccServiceNetworkingPeeredDNSDomain_basic You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=232708 |
Running the new test in CI: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/232720 |
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 - test failure seems to be related to hashicorp/terraform-provider-google#9988 (Apigee instance permadiff)
Given the basic test for this resource failed on its first nightly, can we revert and resolve the Apigee instance failure prior to merging? |
@rileykarson sure, if that's preferred. |
A fix-forward also works, I'd like to avoid merging a resource whose sole test is already failing out of the gate is all! |
given the holidays, I think a rollback is going to be safer / more useful than a fix-forward. |
Adding Apigee NAT addresses for Apigee Instances as per the Apigee API
fixes hashicorp/terraform-provider-google#9358
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)