-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added code for Apigee Environments #4553
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. @melinath, 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 ( 6 files changed, 769 insertions(+), 2 deletions(-)) |
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:
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 769 insertions(+), 2 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 769 insertions(+), 2 deletions(-)) |
/gcbrun just in case it's needed |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=175175" |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 769 insertions(+), 2 deletions(-)) |
I forgot that the tests for this need to be run manually because VCR tests don't run on PRs. First build failed: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloud_ProviderGoogleCloudMmUpstream/175182 |
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 LGTM from a code perspective, assuming the tests pass; it looks like there are some other fields that won't be supported yet, but they can always be added in separate PRs later.
parameters: | ||
- !ruby/object:Api::Type::String | ||
name: 'orgId' |
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 the API docs specify this as parent
- why does orgId
work? https://cloud.google.com/apigee/docs/reference/apis/apigee/rest/v1/organizations.environments/create
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.
parent is just a URL parameter, so we can call it however we want. We actually have the two models in other resources. For example, the older google_access_context_manager_access_level
used parent as is, while the newer google_kms_crypto_key
uses the parent as key_ring
. The second (with an explicit field name that matches what is expected) seems more readable.
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccServiceUsageConsumerQuotaOverride_regionConsumerQuotaOverrideExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=175279" |
Build succeeded: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloud_ProviderGoogleCloudMmUpstream/175293 The failing test is unrelated. |
Added code for Apigee Environments
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)