Introduce a google_kms_key_ring data source #2804
Conversation
…xisting getProject pattern.
| } | ||
|
|
||
| output "key_self_link" { | ||
| value = "${data.google_kms_secret.info.self_link}" |
There was a problem hiding this comment.
Did you mean to add an example of using data.google_kms_key_ring here instead of the kms_secret?
| @@ -51,3 +55,33 @@ func toBool(attribute string) (bool, error) { | |||
| } | |||
| return strconv.ParseBool(attribute) | |||
| } | |||
|
|
|||
| func testAccDataSourceMatchesResourceCheck(dataSourceName string, resourceName string, attrsToCheck []string) resource.TestCheckFunc { | |||
There was a problem hiding this comment.
We use the testAccDataSource* prefix to indicate which resource/datasource that method is providing a config for and generally what file it exists in. For general purpose test helpers we don't use such verbose prefixes. Can you change to something without the testAcc prefix? Perhaps something like checkDataSourceMatchesResource.
| return func(s *terraform.State) error { | ||
| ds, ok := s.RootModule().Resources[dataSourceName] | ||
| if !ok { | ||
| return fmt.Errorf("root module has no resource called %s", dataSourceName) |
There was a problem hiding this comment.
Can you reword the error here and on line 68? I think this should say "module has no data source" instead of "resource" and the second error should be explicit that it can't find the "Resource"
| projectOrg := getTestOrgFromEnv(t) | ||
| billingAccount := getTestBillingAccountFromEnv(t) | ||
| keyRingName := fmt.Sprintf("tf-test-%s", acctest.RandString(10)) | ||
| folderId := os.Getenv("GOOGLE_FOLDER_ID") |
There was a problem hiding this comment.
We don't currently have a pattern for running tests in many configurations based on environment variables. If we need test 2 different configurations of a resource we have to create 2 separate tests. So as this test is encoded it will only ever run 1 of these paths in our CI system. Can you remove the logic for the folder test as it will be dead code for our CI system and anybody who doesn't have that variable set?
Normally I would encourage 2 separate tests, but in this case I think testing both project configurations is a more of a google_project test than a KMS data source test.
|
First of all, thank you for adding this! Also I added a quick commit to get the tests to pass but thanks for making that method more general purpose. Let me know when you've addressed the comments and I'll get this into our next build. |
|
Duplicated by #2891 |
|
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
This introduces a
google_kms_key_ringdata source that allows you to pull in KMS key ring information based on location and name.I've also made a more generic
testAccDataSourceMatchesResourceCheckfunction based on the one that was in thegoogle_projectdata source. Due to the way my GCP organization is structured I've also introduced logic to allowfolder_idto be used instead oforg_idin my test.I've tried running acceptance tests locally, but my GCP organization is structure in a way that's preventing me from running them. I'm PR'ing this now in the hopes that acceptance tests will run on the PR.
In the meantime, I need to figure out how to fix this error, which I'm 99% sure is unrelated to anything in this PR: