datasource: added service account and service account key #1535
Conversation
Added datasources for; service account and service account key. Both datasources should be somewhat similar to their resource counterparts, but they accept a much more flexible 'account_id' and 'service_account_id'.
| Read: dataSourceGoogleServiceAccountRead, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| // Required |
There was a problem hiding this comment.
The comments here aren't necessary since they duplicate the code below, and it's generally good practice not to include comments that duplicate code. Thanks for putting the schema elements in our preferred order, though!
There was a problem hiding this comment.
No worries! I was trying to keep it similar to other datasource code, but I will remove it :)
| d.Set("email", sa.Email) | ||
| d.Set("unique_id", sa.UniqueId) | ||
| d.Set("project", sa.ProjectId) | ||
| d.Set("account_id", strings.Split(sa.Email, "@")[0]) |
There was a problem hiding this comment.
Probably not ideal to set account_id - users will be a little surprised if account_id changes from the value they set it to, on the off chance they choose to interpolate it. In a resource this would be a permanent diff (unless we had a DiffSuppressFunc), which we try to avoid. I'm 50/50 on this - what do you think?
There was a problem hiding this comment.
Yes, I agree, it is a little confusing. I was trying to keep the data set to be the same as the resource google_service_account (which sets the account_id the same way) - but happy to remove it if it is likely to cause confusion?
There was a problem hiding this comment.
Hm. That must have a DiffSuppressFunc, then? I suppose it's better to maintain consistency.
There was a problem hiding this comment.
OK, I was misusing the service account id for this, the service account resource only accepts a service account id, so I have changed the datasource to mimic this; as such the account_id should always be the same. The resource for service account still has this line, so I left it in, but if it is confusing I can remove it?
| Read: dataSourceGoogleServiceAccountKeyRead, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| // Required |
There was a problem hiding this comment.
Same comment - probably best to remove duplicate comments.
| serviceAccountID := d.Get("service_account_id").(string) | ||
|
|
||
| // If the service account id isn't already the fully qualified name | ||
| if !strings.HasPrefix(serviceAccountID, "projects/") { |
There was a problem hiding this comment.
How much of this can be shared? Possibly even with the resources?
There was a problem hiding this comment.
Yup, I can share it with the resource google_sevice_account_key, but I don't know if I can do the same with google_service_account as it has vadliateRFC1035Name(6, 30) in the schema validation for account_id which doesn't allow '@' or /
| Config: testAccDatasourceGoogleServiceAccountKey(account), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| testAccCheckGoogleServiceAccountKeyExists(resourceName), | ||
| resource.TestCheckResourceAttrSet(resourceName, "name"), |
There was a problem hiding this comment.
It'd be great to test that they're set to the right values, also.
There was a problem hiding this comment.
The service account key endpoint returns the name as projects/PROJECT-ID/serviceAccounts/SA-NAME@PROJECT-ID.iam.gserviceaccount.com/keys/<key_id> - should I check that it starts with the expected service account?
There was a problem hiding this comment.
Well, right now I think this just checks that the resource has any attribute as name - if you could check that it's the value you expect somehow, that would help.
Cleaned up some CR comments. 'datasource google_service_account' now behaves more similar to 'resource google_service_account' in regards to how they both accept ONLY the service account id, so there is no weird diffs or confusing behaviour. Moved the service account FQN into a function that 'resource service_account_key' now uses as well. Test that 'datasource google_service_account_key' returns the name that starts with the service account name. And removed now pointless tests that were testing features that were incompatible with the service account resource.
|
Okay, that looks all right to me. I'll run the tests! |
|
Hm. I re-ran the tests (forgot about the resource tests) and now they're crashing with a tooling error. Let me see what's going on there! |
|
Eh, you know what, I ran them locally and I'm sure it works. Merging! |
|
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
Added datasources for;
service accountandservice account key. Both datasources should be somewhat similar to their resource counterparts, but they accept a much more flexibleaccount_idandservice_account_id.This is my first time with acceptance tests (or a datasource), so please let me know if I need to add more, or have done something weird :)