-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 ACM policies fine grained resources #8038
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. Terraform GA: Diff ( 10 files changed, 2875 insertions(+), 544 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 ( 10 files changed, 2897 insertions(+), 556 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 ( 10 files changed, 2897 insertions(+), 557 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 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy|TestAccAlloydbCluster_missingLocation|TestAccAlloydbBackup_missingLocation|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccComputeFirewallPolicyRule_multipleRules|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataSourceAlloydbLocations_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
If we make 2 The perimeter will not be patched by the second |
...rty/terraform/tests/resource_access_context_manager_service_perimeter_ingress_policy_test.go
Show resolved
Hide resolved
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 ( 10 files changed, 2900 insertions(+), 558 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 12 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy|TestAccSqlUser_postgresAbandon|TestAccSqlUser_postgres|TestAccSqlUser_postgresIAM|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccAlloydbBackup_missingLocation|TestAccAlloydbCluster_missingLocation|TestAccDataSourceAlloydbLocations_basic|TestAccDataSourceGoogleFirebaseAndroidAppConfig |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these 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 ( 10 files changed, 2860 insertions(+), 559 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 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccDataSourceAlloydbLocations_basic|TestAccDataSourceGoogleFirebaseAndroidAppConfig |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these 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 ( 10 files changed, 2874 insertions(+), 560 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 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy|TestAccDataSourceAlloydbLocations_basic|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
Can we also add a warning to the top of the old egress/ingress policy resources that these resources are deprecated and points to the new ones?
or query against a BigQuery dataset). | ||
autogen_async: true | ||
exclude_validator: true | ||
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter/IngressPolicy |
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.
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter/IngressPolicy | |
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter |
or actions they match using the ingressTo field. | ||
autogen_async: true | ||
exclude_validator: true | ||
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter/IngressPolicy |
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.
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter/IngressPolicy | |
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter |
perimeter. If left unspecified, then members of `identities` field will | ||
be allowed access. | ||
values: | ||
- :IDENTITY_TYPE_UNSPECIFIED |
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.
- :IDENTITY_TYPE_UNSPECIFIED |
this had a default value of ANY_IDENTITY
right?
perimeter. If left unspecified, then members of `identities` field will be | ||
allowed access. | ||
values: | ||
- :IDENTITY_TYPE_UNSPECIFIED |
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.
- :IDENTITY_TYPE_UNSPECIFIED |
this had a default value of ANY_IDENTITY
right?
Looks like there are couple lint problems, I fixed those that are related to this 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 ( 10 files changed, 2874 insertions(+), 560 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 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccMonitoringNotificationChannel_updateSensitiveLabels|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccDataSourceAlloydbLocations_basic |
@@ -11,6 +11,7 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
# This resource has been deprecated, please refer to ServicePerimeterEgressPolicy. |
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.
Apologies, I should have been more specific. I think we should replace the description
of these resources with a warning that it has been deprecated, and tell the user to go to the google_access_context_manager_service_perimeter_*_policy
equivalent. My intention is that the registry documentation gets changed so that any users wanting to lookup the ingress/egress policy TF documentation get redirected to the correct ones.
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.
Actually, better yet, let's skip the docs entirely for these resources. Using skip_docs: 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.
Looks like skip_docs
is an attribute for skipping the examples. Do we have another attribute for skipping docs entirely? Otherwise I can change in the description.
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.
Ah, I confused that one then. It looks like we do not have an attribute. Changing the description is fine.
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.
Cool, updated
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these 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 ( 12 files changed, 2878 insertions(+), 577 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. |
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 pending final tests.
Tests analyticsTotal tests: Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccDataSourceAlloydbLocations_basic|TestAccDataSourceGoogleFirebaseAndroidAppConfig |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
* Initial commit for ACM policies fine grained resources * Delete old test files * Fix tests failures * Fix tests * Make ingress_policy an array * Change the top level field to be ingressFrom and ingressTo * Support creation for multiple ingress/egress policies * Remove Wrong values in ingress_from/egress_from fields * Fix Lint * Change description for deprecated resources
* Initial commit for ACM policies fine grained resources * Delete old test files * Fix tests failures * Fix tests * Make ingress_policy an array * Change the top level field to be ingressFrom and ingressTo * Support creation for multiple ingress/egress policies * Remove Wrong values in ingress_from/egress_from fields * Fix Lint * Change description for deprecated resources
* Initial commit for ACM policies fine grained resources * Delete old test files * Fix tests failures * Fix tests * Make ingress_policy an array * Change the top level field to be ingressFrom and ingressTo * Support creation for multiple ingress/egress policies * Remove Wrong values in ingress_from/egress_from fields * Fix Lint * Change description for deprecated resources
* Initial commit for ACM policies fine grained resources * Delete old test files * Fix tests failures * Fix tests * Make ingress_policy an array * Change the top level field to be ingressFrom and ingressTo * Support creation for multiple ingress/egress policies * Remove Wrong values in ingress_from/egress_from fields * Fix Lint * Change description for deprecated resources
Initial commit for ACM policies fine grained resources
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)