Allow import steps in beta IAM resource tests - failing tests #6580
Conversation
|
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @c2thorn, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 11 files changed, 441 insertions(+), 58 deletions(-)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
Error from Corresponds to this template code ImportStateId: fmt.Sprintf("<%= import_url -%> <%= object.iam_policy.allowed_iam_role -%> %s"<% unless import_qualifiers.empty? -%>, <% end -%><%= import_qualifiers.join(', ') -%>, <%= example.primary_resource_name -%>, context["condition_title"]),producing this Go code: ImportStateId: fmt.Sprintf("projects/%s/global/machineImages/%s roles/compute.admin %s", getTestProjectFromEnv(), , context["condition_title"]),Looks like the ga-only logic is needed due to beta versions lacking data like This appears to only be true for some resources, so I think it may be a case of adding the missing data to the terraform.yaml files for the affected resources instead of closing this PR. I'll follow this up later... |
SarahFrench
force-pushed
the
iam-test-template-rm-ga-only-logic
branch
from
bef688b to
9ff2989
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. Terraform Beta: Diff ( 11 files changed, 477 insertions(+), 83 deletions(-)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
|
Rebased this PR as it's quite old (3-4months) |
|
API Gateway tests are being more of a pain - need to provide parent resource name when building the |
|
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 Beta: Diff ( 11 files changed, 395 insertions(+)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
…dded to vars by including a hyphen in their value, add missing `primary_resource_name` properties `primary_resource_name` values need to pass both the name of a parent resource and the name of the primary resource, as both are used to template out the full id of the resource. Values of vars need to have `-` in them to trigger `tf-test-` being added to the start. Random string is added to the end automatically too. Making this uniform was important to know the value to define in the `primary_resource_name` values
|
In the most recent commit I had a go fixing the acceptance tests that fail the 'new' test code (that's now included due to removing ga-only logic). I changed the names and values of a bunch of magic-modules/mmv1/provider/terraform/examples.rb Lines 204 to 217 in 4c59a62 It prepends |
|
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 ( 3 files changed, 8 insertions(+), 8 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeMachineImageIamPolicyGenerated_withCondition|TestAccComputeMachineImageIamPolicyGenerated|TestAccComputeMachineImageIamBindingGenerated|TestAccComputeMachineImageIamBindingGenerated_withCondition|TestAccComputeMachineImageIamMemberGenerated|TestAccComputeMachineImageIamMemberGenerated_withCondition|TestAccBigqueryDatapolicyDataPolicyIamMemberGenerated|TestAccBigqueryDatapolicyDataPolicyIamPolicyGenerated|TestAccApiGatewayGatewayIamMemberGenerated|TestAccApiGatewayGatewayIamPolicyGenerated|TestAccApiGatewayGatewayIamBindingGenerated|TestAccApiGatewayApiIamPolicyGenerated|TestAccApiGatewayApiIamMemberGenerated|TestAccApiGatewayApiIamBindingGenerated|TestAccApiGatewayApiConfigIamPolicyGenerated|TestAccApiGatewayApiConfigIamMemberGenerated|TestAccApiGatewayApiConfigIamBindingGenerated|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccApiGatewayApiConfig_apigatewayApiConfigGrpcFullExample|TestAccApiGatewayApiConfig_apigatewayApiConfigGrpcExample|TestAccApiGatewayApiConfig_apigatewayApiConfigFullExample|TestAccApiGatewayApiConfig_apigatewayApiConfigBasicExample|TestAccApiGatewayGateway_apigatewayGatewayFullExample|TestAccApiGatewayGateway_apigatewayGatewayBasicExample|TestAccServiceDirectoryServiceIamPolicyGenerated|TestAccServiceDirectoryNamespaceIamMemberGenerated|TestAccServiceDirectoryNamespaceIamBindingGenerated|TestAccApiGatewayApi_apigatewayApiBasicExampleUpdated|TestAccServiceDirectoryServiceIamMemberGenerated|TestAccServiceDirectoryServiceIamBindingGenerated|TestAccServiceDirectoryNamespaceIamPolicyGenerated|TestAccRuntimeConfigConfigIamPolicyGenerated|TestAccApiGatewayApi_apigatewayApiFullExample|TestAccApiGatewayApi_apigatewayApiBasicExample|TestAccDataprocMetastoreFederationIamPolicyGenerated|TestAccDataprocMetastoreFederationIamMemberGenerated|TestAccApiGatewayApiConfig_apigatewayApiConfigBasicExampleUpdated|TestAccDataCatalogTaxonomyIamMemberGenerated|TestAccDataCatalogTaxonomyIamBindingGenerated|TestAccDataCatalogPolicyTagIamPolicyGenerated|TestAccDataCatalogPolicyTagIamMemberGenerated|TestAccApiGatewayGateway_apigatewayGatewayBasicExampleUpdated|TestAccDataprocMetastoreFederationIamBindingGenerated|TestAccDataCatalogTaxonomyIamPolicyGenerated|TestAccDataCatalogPolicyTagIamBindingGenerated |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
|
At least the code builds now! |
…vicedirectory Service and Namespace resources
|
Service directory IAM acceptance tests are a pain to update - around use of Have issues with URL that repeats like |
|
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 ( 5 files changed, 16 insertions(+), 20 deletions(-)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
…all needs 1 arg but has 2 args`
|
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 ( 5 files changed, 20 insertions(+), 20 deletions(-)) |
|
Import of In the example below you set resource "google_data_catalog_taxonomy" "basic_taxonomy" {
provider = google-beta
region = "us"
display_name = "tf_test_my_display_name"
description = "A collection of policy tags"
activated_policy_types = ["FINE_GRAINED_ACCESS_CONTROL"]
}Given the HCL above, instead of having a name like this: instead it's like this: Where the number is returned from the API. Because of this, we would need to get data out of the test to configure the next test step that tests imports, and currently this isn't possible |
|
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 ( 8 files changed, 31 insertions(+), 28 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccRegionInstanceGroupManager_stateful|TestAccContainerCluster_failedCreation|TestAccServiceDirectoryNamespaceIamBindingGenerated|TestAccServiceDirectoryServiceIamPolicyGenerated|TestAccServiceDirectoryServiceIamMemberGenerated|TestAccServiceDirectoryServiceIamBindingGenerated|TestAccServiceDirectoryNamespaceIamMemberGenerated|TestAccDataCatalogTaxonomyIamPolicyGenerated|TestAccDataCatalogTaxonomyIamMemberGenerated|TestAccDataCatalogPolicyTagIamPolicyGenerated|TestAccDataCatalogTaxonomyIamBindingGenerated|TestAccDataCatalogPolicyTagIamBindingGenerated|TestAccServiceDirectoryNamespaceIamPolicyGenerated|TestAccDataCatalogPolicyTagIamMemberGenerated|TestAccFirebaserulesRelease_BasicRelease |
|
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: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 8 files changed, 32 insertions(+), 26 deletions(-)) |
|
Breaking change is expected - I'm going to get feedback from my reviewer about how to address the IAM tests where code generate doesn't seem to place nicely with |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccContainerCluster_failedCreation|TestAccServiceDirectoryServiceIamBindingGenerated|TestAccServiceDirectoryServiceIamPolicyGenerated|TestAccServiceDirectoryNamespaceIamMemberGenerated|TestAccServiceDirectoryServiceIamMemberGenerated|TestAccServiceDirectoryNamespaceIamPolicyGenerated|TestAccDataCatalogPolicyTagIamPolicyGenerated|TestAccDataCatalogPolicyTagIamBindingGenerated|TestAccDataCatalogPolicyTagIamMemberGenerated|TestAccDataCatalogTaxonomyIamPolicyGenerated|TestAccDataCatalogTaxonomyIamMemberGenerated|TestAccDataCatalogTaxonomyIamBindingGenerated|TestAccFirebaserulesRelease_BasicRelease |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
|
☝️ The breaking changes make the tests pass the import steps, but it's a breaking change. I'm prepared to revert bfcde13 but I don't know what the 'proper' solution is. I'm a bit stumped about how to make the Also, the |
| primary_resource_name: "fmt.Sprintf(\"tf-test-api-%s\", context[\"random_suffix\"])" | ||
| vars: | ||
| name: "api" | ||
| api_id: "api-" |
There was a problem hiding this comment.
Note: dangling dash characters are here to trigger logic that automatically adds tf-test- at the start of the value in the templated HCL used in the test. Having a - present is necessary for this.
There was a problem hiding this comment.
yeah, this is an unfortunate "feature". one way to make this look cleaner is to add a prefix like my-api
There was a problem hiding this comment.
Yeah that's a good idea, I'll push a commit making these vars have prefixes after discussion about the issue(s) with ServiceDirectory tests is concluded - I don't want to make the PR timeline more disjointed!
There was a problem hiding this comment.
👍 Alternately / additionally, I'd be happy to approve the straightforward bits in a separate PR while we sort out the complicated bits here.
There was a problem hiding this comment.
That sounds like the better solution - this PR ballooned a lot from the original intention
There was a problem hiding this comment.
Here's the new PR - it contains everything in this PR and leaves servicedirectory and datacatalog untouched other than changes needed to let the new import steps in those tests build (pass correct number of args into fmt.Sprintf).
There are failing VCR tests for servicedirectory and datacatalog, but those would be addressed in this PR
| parent_resource_attribute: 'name' | ||
| parent_resource_attribute: 'namespace_id' | ||
| method_name_separator: ':' | ||
| fetch_iam_policy_verb: :POST | ||
| set_iam_policy_verb: :POST | ||
| base_url: "projects/{{project}}/locations/{{location}}/namespaces/{{namespace_id}}" | ||
| import_format: ["projects/{{project}}/locations/{{location}}/namespaces/{{namespace_id}}"] |
There was a problem hiding this comment.
This is a breaking change 💔
The error that happens with the old code is that there's a 404 when trying to get the IAM policy on the Namespace:
POST /v1beta1/tf-test-example-namespace6agdt0mwhx:getIamPolicy?alt=json
This appears to be due to the qualifyNamespaceUrl function producing the wrong URL that lacks project/location etc info. I tried updating the test to have a long-form name so the URL is complete but it didn't work for me and I figured it was worth getting some advice!
There was a problem hiding this comment.
yeah, this is definitely a breaking change for users of this resource. Basically to justify this change in a minor release we need to be reasonably sure that if someone was using a configuration with name it just wouldn't work for them - is that the case here? Like, can these IAM resources be used to set the IAM policy but then not get or update it, or do they just not work at all?
There was a problem hiding this comment.
Just to double-check, does setting base_url & import_format to just contain {{name}} work?
There was a problem hiding this comment.
Just to double-check, does setting base_url & import_format to just contain {{name}} work?
When this is set for the Namespace resource and I run the TestAccServiceDirectoryNamespaceIamMemberGenerated test it fails on the first step (test_step_number=1 mentioned in the copy/paste output below) because it's making a POST request to /v1beta1/projects:getIamPolicy?alt=json instead of /v1beta1/projects/XXX/locations/XXX/namespaces/tf-test-xxx:getIamPolicy?alt=json
2023-01-27T17:28:41.786Z [WARN] sdk.helper_resource: Error running Terraform CLI command: test_step_number=1
error=
| exit status 1
|
| Error: Error retrieving IAM policy for servicedirectory namespace "projects": googleapi: got HTTP response code 404 with body: <!DOCTYPE html>
| <html lang=en>
| <meta charset=utf-8>
| <meta name=viewport content="initial-scale=1, minimum-scale=1, width=device-width">
| <title>Error 404 (Not Found)!!1</title>
| <style>
| *{margin:0;padding:0}html,code{font:15px/22px arial,sans-serif}html{background:#fff;color:#222;padding:15px}body{margin:7% auto 0;max-width:390px;min-height:180px;padding:30px 0 15px}* > body{background:url(//www.google.com/images/errors/robot.png) 100% 5px no-repeat;padding-right:205px}p{margin:11px 0 22px;overflow:hidden}ins{color:#777;text-decoration:none}a img{border:0}@media screen and (max-width:772px){body{background:none;margin-top:0;max-width:none;padding-right:0}}#logo{background:url(//www.google.com/images/branding/googlelogo/1x/googlelogo_color_150x54dp.png) no-repeat;margin-left:-5px}@media only screen and (min-resolution:192dpi){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat 0% 0%/100% 100%;-moz-border-image:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) 0}}@media only screen and (-webkit-min-device-pixel-ratio:2){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat;-webkit-background-size:100% 100%}}#logo{display:inline-block;height:54px;width:150px}
| </style>
| <a href=//www.google.com/><span id=logo aria-label=Google></span></a>
| <p><b>404.</b> <ins>That’s an error.</ins>
| <p>The requested URL <code>/v1beta1/projects:getIamPolicy?alt=json</code> was not found on this server. <ins>That’s all we know.</ins>
|
|
| with google_service_directory_namespace_iam_member.foo,
| on terraform_plugin_test.tf line 14, in resource "google_service_directory_namespace_iam_member" "foo":
| 14: resource "google_service_directory_namespace_iam_member" "foo" {
There was a problem hiding this comment.
Basically to justify this change in a minor release we need to be reasonably sure that if someone was using a configuration with name it just wouldn't work for them - is that the case here? Like, can these IAM resources be used to set the IAM policy but then not get or update it, or do they just not work at all?
I manually tested creating a google_service_directory_namespace and a google_service_directory_namespace_iam_binding resource and can create and update the iam_binding without any errors. I'm not sure why there isn't a 404 like in the tests 😕
Something that doesn't seem to work is import of google_service_directory_namespace_iam_binding:
resource "google_service_directory_namespace" "example" {
provider = google-beta
namespace_id = "tf-test-example-namespace-sarahfrench"
location = "us-central1"
labels = {
key = "value"
foo = "bar"
}
}
resource "google_service_directory_namespace_iam_binding" "foo" {
provider = google-beta
name = google_service_directory_namespace.example.name
role = "roles/viewer"
members = ["user:[email protected]"]
}After applying the plan to create these I then remove it from state and attempt to import the binding following the provider docs and it fails:
terraform state rm google_service_directory_namespace_iam_binding.foo
# works
terraform import google_service_directory_namespace_iam_binding.foo "projects/[PROJECT_ID]/locations/us-central1/namespaces/tf-test-example-namespace-sarahfrench roles/viewer"
# Error: Import id "" doesn't match any of the accepted formats: [projects/(?P<project>[^/]+)/locations/(?P<location>[^/]+)/namespaces/(?P<namespace_id>[^/]+) (?P<project>[^/]+)/(?P<location>[^/]+)/(?P<namespace_id>[^/]+) (?P<location>[^/]+)/(?P<namespace_id>[^/]+)]
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 8 files changed, 32 insertions(+), 26 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccRegionInstanceGroupManager_stateful|TestAccContainerCluster_failedCreation|TestAccServiceDirectoryServiceIamBindingGenerated|TestAccServiceDirectoryServiceIamPolicyGenerated|TestAccDataCatalogTaxonomyIamPolicyGenerated|TestAccDataCatalogPolicyTagIamPolicyGenerated|TestAccServiceDirectoryServiceIamMemberGenerated|TestAccDataCatalogTaxonomyIamMemberGenerated|TestAccDataCatalogTaxonomyIamBindingGenerated|TestAccDataCatalogPolicyTagIamMemberGenerated|TestAccDataCatalogPolicyTagIamBindingGenerated |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
SarahFrench
changed the title
…or affected datacatalog resources
|
Latest commit closes hashicorp/terraform-provider-google#13667 |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 8 files changed, 32 insertions(+), 26 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccRegionInstanceGroupManager_stateful|TestAccComputeForwardingRule_update|TestAccApigeeAddonsConfig_apigeeAddonsTestExample|TestAccServiceDirectoryServiceIamBindingGenerated|TestAccServiceDirectoryServiceIamPolicyGenerated|TestAccServiceDirectoryServiceIamMemberGenerated|TestAccDataSourceDnsRecordSet_basic|TestAccFrameworkProviderMeta_setModuleName|TestAccCloudbuildv2ConnectionIamPolicyGenerated|TestAccCloudbuildv2ConnectionIamMemberGenerated|TestAccCloudbuildv2ConnectionIamBindingGenerated |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
|
Converting to draft as this PR is eventually going to be closed, after replacement with PRs for:
|
|
Closing this PR in favour of:
|
3 participants
Closes hashicorp/terraform-provider-google#12610
also, closes hashicorp/terraform-provider-google#13667
The main change in this PR is removing this GA-only logic in the test template
mmv1/templates/terraform/examples/base_configs/iam_test_file.go.erbThis change introduces import steps into beta IAM resources' tests, and a lot of those tests fail because of missing data in the YAML.
As a result of those failing tests I have added missing data where I can and I've renamed some variables for clarity and consistency.
If this PR is for Terraform, I acknowledge that I have:
make testandmake lintto ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)