-
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
Compute public advertised prefixes #7682
Conversation
Oops! It looks like you're using unknown release-note types in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
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. Terraform GA: Diff ( 9 files changed, 1515 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Errors occurred during REPLAYING 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. Terraform GA: Diff ( 9 files changed, 1345 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
/gcbrun |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
As noted, tested fields aren't being captured correctly due to the handwritten tests.
|
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. Terraform GA: Diff ( 9 files changed, 1345 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
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. Terraform GA: Diff ( 9 files changed, 1345 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputePublicPrefixes|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccDataSourceGoogleFirebaseAndroidAppConfig |
Tests passed during RECORDING mode: All tests passed |
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! Only small comments
name: 'region' | ||
description: 'A region where the prefix will reside.' | ||
url_param_only: true | ||
immutable: true |
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.
Do we still need to set it explicitly, given the resource is immutable?
name: 'parentPrefix' | ||
description: The URL of parent prefix. Either PublicAdvertisedPrefix or PublicDelegatedPrefix. | ||
required: true | ||
immutable: true |
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.
also here
@shuyama1: Want to pick this up while Sam's OOO? I can review the delta. |
Sure! Taking a look now |
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. Terraform GA: Diff ( 9 files changed, 1345 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccDataSourceGoogleFirebaseAndroidAppConfig |
Tests passed during RECORDING mode: All tests passed |
FYI @slevenick I added backticks for the resource names: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/.ci/RELEASE_NOTES_GUIDE.md#unticked-resource-names |
Adds PublicAdvertisedPrefix and PublicDelegatedPrefix resources. These are immutable though they have a PATCH method in the API because the PATCH method is only allowed to act on members of the
publicDelegatedPrefixs
field which are better represented as their own resources.Creating a PAP requires reverse DNS verification, so we use a special loopback IP range and special description (encrypted so as not to publicize this test-only value) to create a test resource. Because we use a specific IP range and quotas for PAPs are very low the tests are handwritten and run in serial to prevent conflict.
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)