Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix service account key data source name #1932

Merged
merged 6 commits into from
Aug 24, 2018

Conversation

emilymye
Copy link
Contributor

@emilymye emilymye commented Aug 23, 2018

Fixes #1909

$ make testacc TEST=./google TESTARGS='--run="TestAccDatasourceGoogleServiceAccountKey"'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v --run="TestAccDatasourceGoogleServiceAccountKey" -timeout 120m
=== RUN TestAccDatasourceGoogleServiceAccountKey_basic
=== PAUSE TestAccDatasourceGoogleServiceAccountKey_basic
=== RUN TestAccDatasourceGoogleServiceAccountKey_errors
=== PAUSE TestAccDatasourceGoogleServiceAccountKey_errors
=== CONT TestAccDatasourceGoogleServiceAccountKey_basic
=== CONT TestAccDatasourceGoogleServiceAccountKey_errors
--- PASS: TestAccDatasourceGoogleServiceAccountKey_errors (14.38s)
--- PASS: TestAccDatasourceGoogleServiceAccountKey_basic (18.11s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 18.281s

@ghost ghost added the size/l label Aug 23, 2018
@emilymye emilymye requested a review from danawillow August 23, 2018 00:03
@@ -50,7 +94,39 @@ resource "google_service_account_key" "acceptance" {
public_key_type = "TYPE_X509_PEM_FILE"
}

data "google_service_account_key" "acceptance" {
name = "${google_service_account_key.acceptance.id}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really really want to get us away from the practice of relying on the way an id is shaped, since it limits our availability to change them in the future. I just filed #1927 yesterday about that, but in the meantime, is there something else that would work here?

)

func dataSourceGoogleServiceAccountKey() *schema.Resource {
return &schema.Resource{
Read: dataSourceGoogleServiceAccountKeyRead,

Schema: map[string]*schema.Schema{
"service_account_id": &schema.Schema{
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs need updating too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

@ghost ghost added the size/l label Aug 23, 2018
@emilymye
Copy link
Contributor Author

Reran tests:

$ make testacc TEST=./google TESTARGS='--run="TestAccDatasourceGoogleServiceAccountKey"'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v --run="TestAccDatasourceGoogleServiceAccountKey" -timeout 120m
=== RUN TestAccDatasourceGoogleServiceAccountKey_basic
=== PAUSE TestAccDatasourceGoogleServiceAccountKey_basic
=== RUN TestAccDatasourceGoogleServiceAccountKey_errors
=== PAUSE TestAccDatasourceGoogleServiceAccountKey_errors
=== CONT TestAccDatasourceGoogleServiceAccountKey_basic
=== CONT TestAccDatasourceGoogleServiceAccountKey_errors
--- PASS: TestAccDatasourceGoogleServiceAccountKey_errors (13.86s)
--- PASS: TestAccDatasourceGoogleServiceAccountKey_basic (17.85s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google

fullKeyName = keyFromSAId
}

if r.MatchString(fullKeyName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, what about adding a ValidateFunc to "name" so these are caught plan-time? That would let us just return the name a bit early from this if it's set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going add ValidateFunc to the schema but also leave this here since interpolated values don't trigger ValidateFunc.

@@ -51,6 +95,38 @@ resource "google_service_account_key" "acceptance" {
}

data "google_service_account_key" "acceptance" {
service_account_id = "${google_service_account_key.acceptance.id}"
name = "${google_service_account_key.acceptance.id}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you change to .name here too?

output "mykey_public_key" {
value = "${data.google_service_account_key.mykey.public_key}"
data "google_service_account_key" "mykey" {
name = "${google_service_account_key.mykey.name}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bleh this example is super contrived, although I don't think there are any other resources that output keys that you could input into the data source so I don't have any other ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah...yeah.

@ghost ghost added the size/l label Aug 24, 2018
Optional: true,
ConflictsWith: []string{"name"},
Deprecated: "Please use name to specify full service account key path projects/{project}/serviceAccounts/{serviceAccount}/keys/{keyId}",
ValidateFunc: validateRegexp(ServiceAccountKeyNameRegex),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one probably shouldn't have the validate because if someone was (somehow) using it successfully before, now it'll start failing. I'm not sure whether this data source was really ever usable, but just in case.

fullKeyName = keyFromSAId
}

log.Printf("[DEBUG] FULL KEY NAME: %q %q %q", fullKeyName, keyName, keyFromSAId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can probably go away :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops

return fullKeyName, nil
}

return "", fmt.Errorf("invalid key name %q does not match regexp %q", fullKeyName, ServiceAccountKeyNameRegex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also maybe add a note that we're doing apply-time validation here because validatefuncs won't get called at plan-time for interpolated values

@emilymye
Copy link
Contributor Author

$make testacc TEST=./google TESTARGS='--run="TestAccDatasourceGoogleServiceAccountKey_"'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v --run="TestAccDatasourceGoogleServiceAccountKey_" -timeout 120m
=== RUN TestAccDatasourceGoogleServiceAccountKey_basic
=== PAUSE TestAccDatasourceGoogleServiceAccountKey_basic
=== RUN TestAccDatasourceGoogleServiceAccountKey_errors
=== PAUSE TestAccDatasourceGoogleServiceAccountKey_errors
=== CONT TestAccDatasourceGoogleServiceAccountKey_basic
=== CONT TestAccDatasourceGoogleServiceAccountKey_errors
--- PASS: TestAccDatasourceGoogleServiceAccountKey_errors (13.75s)
--- PASS: TestAccDatasourceGoogleServiceAccountKey_basic (17.75s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 17.929s

@emilymye emilymye merged commit 7e82a98 into hashicorp:master Aug 24, 2018
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
* fix service account key data source name

* switch id to name

* update docs

* doc format

* fixes for validation and tests

* last fixes for service account key data source
@ghost
Copy link

ghost commented Nov 17, 2018

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 ghost locked and limited conversation to collaborators Nov 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants