Make google_service_account resource importable #606
Conversation
| t.Parallel() | ||
|
|
||
| resourceName := "google_service_account.acceptance" | ||
| project := os.Getenv("GOOGLE_PROJECT") |
There was a problem hiding this comment.
Is there a reason why you decided to set the project id specifically in this test rather than letting the test read it from the environment? If so mind adding a comment to explain?
There was a problem hiding this comment.
Good point, project is indeed an optional parameter so it should work with or without it.
Removing it from the import test raises an issue so I guess we should keep both test cases:
TF_ACC=1 go test ./google/ -v -run=TestAccGoogleServiceAccount_import.* -timeout 120m
=== RUN TestAccGoogleServiceAccount_importBasic
=== RUN TestAccGoogleServiceAccount_importWithProject
--- FAIL: TestAccGoogleServiceAccount_importBasic (2.66s)
testing.go:435: Step 0 error: After applying this step, the plan was not empty:
DIFF:
DESTROY/CREATE: google_service_account.acceptance
account_id: "terraform-1nd79xoges" => "terraform-1nd79xoges"
display_name: "terraform-1nd79xoges" => "terraform-1nd79xoges"
email: "terraform-1nd79xoges@myproject.iam.gserviceaccount.com" => "<computed>"
name: "projects/myproject/serviceAccounts/terraform-1nd79xoges@myproject.iam.gserviceaccount.com" => "<computed>"
project: "myproject" => "" (forces new resource)
unique_id: "113399152587722893408" => "<computed>"
STATE:
google_service_account.acceptance:
ID = projects/myproject/serviceAccounts/terraform-1nd79xoges@myproject.iam.gserviceaccount.com
account_id = terraform-1nd79xoges
display_name = terraform-1nd79xoges
email = terraform-1nd79xoges@myproject.iam.gserviceaccount.com
name = projects/myproject/serviceAccounts/terraform-1nd79xoges@myproject.iam.gserviceaccount.com
project = myproject
unique_id = 113399152587722893408
--- PASS: TestAccGoogleServiceAccount_importWithProject (3.91s)
FAIL
exit status 1
FAIL github.com/terraform-providers/terraform-provider-google/google 3.920s
No sure why yet.
Once fixed, I'll add a comment.
There was a problem hiding this comment.
Adding Computed: True to the google_service_account's project field ensures the project id is always stored in the state and resolves the issue. Both test cases now pass.
I've also added an extra step and some assertions to the TestAccGoogleServiceAccount_basic test case to assert this.
… id is always stored in the state, defined in configuration or not. Add corresponding test cases
|
|
||
| resourceName := "google_service_account.acceptance" | ||
| sa_name := "terraform-" + acctest.RandString(10) | ||
| conf := testAccGoogleServiceAccount_import(sa_name) |
There was a problem hiding this comment.
nit: s/sa_name/saName/
Also nit: this is (and other variables defined here are) only used in one place, so you could pretty easily inline them
| func testAccGoogleServiceAccountWithProject(project, account, name string) string { | ||
| t := `resource "google_service_account" "acceptance" { | ||
| project = "%v" | ||
| account_id = "%v" |
| "google_service_account.acceptance", "project", project), | ||
| ), | ||
| }, | ||
| // The third step adds the default project to the service account |
There was a problem hiding this comment.
Hmm, is this really what you want? Wouldn't this test just update the config to what it already is and then not actually do anything? I guess that's not a huge deal but if that's what you're going for it might be good to explain it in a comment. Otherwise maybe it deserves to be a separate TestCase, rather than a TestStep.
There was a problem hiding this comment.
Well, what I am trying to assert here is that adding the project field to an existing configuration with the same value as the default does not recreate the service account, which was not the case without the Computed: true.
But I realize that this test should rather assert that the service account's unique id does not change between step 2 and 3.
There was a problem hiding this comment.
Test case and comment updated, but not sure if this is the best way to compare state values between steps.
@danawillow, what do you think?
|
Thanks @pdecat! |
* Make google_service_account resource importable * Add google_service_account testcase with default project * Mark google_service_account.project as computed to ensure the project id is always stored in the state, defined in configuration or not. Add corresponding test cases * Inline variables with single usage * Replace tabs with spaces in configuration strings * Ensure service account is not recreated when the default project is explicitely added to the configuration * camelcase
Signed-off-by: Modular Magician <magic-modules@google.com>
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
No description provided.