Adding Terraform resources for Tenancy APIs in GKEHub #8396
Conversation
|
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
|
Hello! I am a robot. It looks like you are a community contributor. @NickElliot, 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. |
|
|
/gcbrun |
|
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 ( 20 files changed, 4510 insertions(+), 3 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. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Action takenFound 10 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccGKEHub2ScopeRBACRoleBinding_gkehubScopeRbacRoleBindingBasicExample|TestAccGKEHub2Scope_gkehubScopeBasicExample|TestAccGKEHub2Namespace_gkehubNamespaceBasicExample|TestAccGKEHub2MembershipRBACRoleBinding_gkehubMembershipRbacRoleBindingBasicExample|TestAccGKEHub2MembershipBinding_gkehubMembershipBindingBasicExample|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules|TestAccGKEHub2ScopeIamMemberGenerated|TestAccGKEHub2ScopeIamBindingGenerated|TestAccGKEHub2ScopeIamPolicyGenerated |
|
Rerun these tests in REPLAYING mode to catch issues
|
There was a problem hiding this comment.
Hey! I've added one small comment that was causing a lint error, but wanted to ask if you could break this PR up into smaller layered ones for each individual resource (or if there are mutually dependent ones, just those that have to exist together). A five resource review ends up being somewhat unsustainable and likely to result in more review passes than them being separate, resulting it taking longer to reach the provider.
|
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 ( 20 files changed, 4510 insertions(+), 3 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. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
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 testsTestAccGKEHub2ScopeRBACRoleBinding_gkehubScopeRbacRoleBindingBasicExample|TestAccGKEHub2Namespace_gkehubNamespaceBasicExample|TestAccGKEHub2MembershipRBACRoleBinding_gkehubMembershipRbacRoleBindingBasicExample|TestAccComputeFirewallPolicyRule_multipleRules|TestAccContainerAwsNodePool_BetaBasicHandWritten |
|
|
|
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 ( 20 files changed, 4510 insertions(+), 3 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. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
1 similar comment
|
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 ( 20 files changed, 4510 insertions(+), 3 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. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
There was a problem hiding this comment.
In case you missed the previous comment: #8396 (review)
Thanks!
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 testsTestAccGKEHub2ScopeRBACRoleBinding_gkehubScopeRbacRoleBindingBasicExample|TestAccGKEHub2Namespace_gkehubNamespaceBasicExample|TestAccGKEHub2MembershipRBACRoleBinding_gkehubMembershipRbacRoleBindingBasicExample|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules |
|
Rerun these tests in REPLAYING mode to catch issues
|
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 testsTestAccGKEHub2ScopeRBACRoleBinding_gkehubScopeRbacRoleBindingBasicExample|TestAccGKEHub2Namespace_gkehubNamespaceBasicExample|TestAccGKEHub2MembershipRBACRoleBinding_gkehubMembershipRbacRoleBindingBasicExample|TestAccSpannerDatabaseIamPolicy|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules|TestAccBigQueryDataTable_bigtable |
|
Rerun these tests in REPLAYING mode to catch issues
|
…hings simpler in the review
|
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 ( 14 files changed, 2861 insertions(+), 3 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. 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 testsTestAccGKEHub2ScopeRBACRoleBinding_gkehubScopeRbacRoleBindingBasicExample|TestAccGKEHub2Namespace_gkehubNamespaceBasicExample|TestAccContainerAwsNodePool_BetaBasicHandWritten |
|
|
There was a problem hiding this comment.
Starting from the top, the namespace test appears to be failing with a 404 which at a glance appears to be due to the URL not being accurate based on what I'm seeing on the documentation. Is there a page I'm not seeing? Or if what I linked in the comment is correct, could you try updating that and running the test locally to see if it passes? Thanks!
|
|
|
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 ( 18 files changed, 3624 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_gke_hub_scope_iam_binding" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
Resource: resource "google_gke_hub_scope_iam_member" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
Resource: resource "google_gke_hub_scope_rbac_role_binding" "primary" {
group = # value needed
}
|
|
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 ( 20 files changed, 3809 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_gke_hub_scope_iam_binding" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
Resource: resource "google_gke_hub_scope_iam_member" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
|
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 testsTestAccGKEHub2MembershipBinding_gkehubMembershipBindingBasicExample_update|TestAccGKEHub2Scope_gkehubScopeBasicExample_update|TestAccGKEHub2ScopeRBACRoleBinding_gkehubScopeRbacRoleBindingBasicExample_update |
|
Rerun these tests in REPLAYING mode to catch issues
|
@NickElliot Could you pinpoint the exact error of the Scope update test(TestAccGKEHub2Scope_gkehubScopeBasicExample_update) to me please? |
There was a problem hiding this comment.
Sorry for the delay, review was sitting in progress before lunch.
TestAccGKEHub2Scope_gkehubScopeBasicExample_update is having a permadiff failure due to all_memberships not maintaining the true value when it attempts to refresh the plan (with the API saying it currently is false), despite returning a success on the PATCH with the true value. It would appear to be a case of go/dont-break-terraform#new-values-returned-for-old-fields, but if the field is actually immutable or output only then the YAML needs to be changed to match. As you're more familiar with it being on the service team in question, the intended API behavior needs to be clarified.
|
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 ( 20 files changed, 3809 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_gke_hub_scope_iam_binding" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
Resource: resource "google_gke_hub_scope_iam_member" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
|
Thank you for sharing that error. I talked to the team and we feel it should be removed at this point. Sorry for the confusion. |
|
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 ( 19 files changed, 3634 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_gke_hub_scope_iam_binding" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
Resource: resource "google_gke_hub_scope_iam_member" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
|
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 testsTestAccGKEHub2Namespace_gkehubNamespaceBasicExample|TestAccGKEHub2ScopeRBACRoleBinding_gkehubScopeRbacRoleBindingBasicExample_update|TestAccGKEHub2ScopeRBACRoleBinding_gkehubScopeRbacRoleBindingBasicExample|TestAccGKEHub2MembershipBinding_gkehubMembershipBindingBasicExample|TestAccGKEHub2MembershipBinding_gkehubMembershipBindingBasicExample_update|TestAccDataSourceGoogleServiceAccountJwt|TestAccDataSourceGoogleServiceAccountAccessToken_basic |
|
Rerun these tests in REPLAYING mode to catch issues
|
|
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 ( 19 files changed, 3624 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_gke_hub_scope_iam_binding" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
Resource: resource "google_gke_hub_scope_iam_member" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
}
|
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAppEngineStandardAppVersion_update|TestAccVPCAccessConnectorDatasource_basic |
|
Rerun these tests in REPLAYING mode to catch issues
|
Fixes 15167
If this PR is for Terraform, I acknowledge that I have:
make testandmake lintin the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)