-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Access Context Manager - Add support for roles in service perimeter resources #13413
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
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: see go/terraform-auto-test-runs to set up automatic test runs. @BBBmau, 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. |
b0eaecf
to
440ef12
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_access_context_manager_service_perimeter" "primary" {
spec {
egress_policies {
egress_to {
roles = # value needed
}
}
ingress_policies {
ingress_to {
roles = # value needed
}
}
}
}
Resource: resource "google_access_context_manager_service_perimeter_dry_run_egress_policy" "primary" {
egress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_dry_run_ingress_policy" "primary" {
ingress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_egress_policy" "primary" {
egress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_ingress_policy" "primary" {
ingress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeters" "primary" {
service_perimeters {
spec {
egress_policies {
egress_to {
roles = # value needed
}
}
ingress_policies {
ingress_to {
roles = # value needed
}
}
}
}
}
|
Tests analyticsTotal tests: 3 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. |
resource "google_access_context_manager_service_perimeter" "test-access" { | ||
parent = "accessPolicies/${google_access_context_manager_access_policy.test-access.name}" | ||
name = "accessPolicies/${google_access_context_manager_access_policy.test-access.name}/servicePerimeters/%s" | ||
resource "google_access_context_manager_service_perimeter" "granular-controls-perimeter" { |
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.
although you've add the root ingress_to
in the test files it's still necessary to add them in the examples also. It appears that it's missing here in the root resource.
*_test.go
is used for update 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.
We disable to the example to test generation since it doesn't behave well with our API. I fixed the customer flattener for one of the resources which fixed the 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.
can you go ahead and still include them in the examples with the them set as skip_test: true
? It should also contain a link or at least an explanation of why this is skipped. Your explanation here would be good.
https://github.com/GoogleCloudPlatform/magic-modules/pull/13413/files#r2012461682
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.
Updated all the examples and added a comment to explain why the tests are disabled.
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.
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.
Added fields to the spec section for that test. Should fix it
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.
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.
Im not sure what else I need to do. This field is being covered in every test we have for every resource that has this field. I also updated all the examples. We always have these missing test reports because I believe it does not pickup our manual tests. See #10990 for an example.
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.
Seeing the PR you linked help a ton. Thanks!
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.
Thanks for the thorough review. We will look into how we can fix our test setup to leverage the recommended generated 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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_access_context_manager_service_perimeter" "primary" {
spec {
egress_policies {
egress_to {
roles = # value needed
}
}
ingress_policies {
ingress_to {
roles = # value needed
}
}
}
}
Resource: resource "google_access_context_manager_service_perimeter_dry_run_egress_policy" "primary" {
egress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_dry_run_ingress_policy" "primary" {
ingress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_egress_policy" "primary" {
egress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_ingress_policy" "primary" {
ingress_to {
roles = # value needed
}
}
|
Tests analyticsTotal tests: 3 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
|
@@ -313,6 +313,14 @@ properties: | |||
is_set: true | |||
item_type: | |||
type: String | |||
- name: 'roles' |
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.
we're getting missing tests due to a create test not being added in the examples list: refer to https://googlecloudplatform.github.io/magic-modules/test/test/#add-a-create-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.
We can not use the examples to generate tests because those tests will run in parallel and all of our tests depend on creating a resource that can only be created once at a time. We are looking to refactor our tests in the future but for now this is what we have. I have added this field to the existing tests for all of our resources.
Hi @BBBmau can you please take a look at this again. We need to have this merged by tomorrow to meet release deadlines. |
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_access_context_manager_service_perimeter" "primary" {
spec {
egress_policies {
egress_to {
roles = # value needed
}
}
ingress_policies {
ingress_to {
roles = # value needed
}
}
}
}
Resource: resource "google_access_context_manager_service_perimeter_dry_run_egress_policy" "primary" {
egress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_dry_run_ingress_policy" "primary" {
ingress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_egress_policy" "primary" {
egress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_ingress_policy" "primary" {
ingress_to {
roles = # value needed
}
}
|
Tests analyticsTotal tests: 3 Click here to see the affected service packages
🟢 All tests passed! View the build log |
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_access_context_manager_service_perimeter_dry_run_egress_policy" "primary" {
egress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_dry_run_ingress_policy" "primary" {
ingress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_egress_policy" "primary" {
egress_to {
roles = # value needed
}
}
Resource: resource "google_access_context_manager_service_perimeter_ingress_policy" "primary" {
ingress_to {
roles = # value needed
}
}
|
Tests analyticsTotal tests: 3 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
|
b2dafef
…esources (GoogleCloudPlatform#13413) Co-authored-by: Charlesleonius <[email protected]>
…esources (GoogleCloudPlatform#13413) Co-authored-by: Charlesleonius <[email protected]>
Add support for IAM roles in Access Context Manager service perimeter resources.