-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add API Keys #4804
Add API Keys #4804
Conversation
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @melinath, please review this PR or find an appropriate assignee. |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=188484" |
There is a bug in the dclclient. It doesn't check if the key exists before getting keyString.
Also, the POST command can't take a name.
|
I don't have a lot of experience with the DCL yet so I'm reassigning review of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, yeah that's definitely wrong. Name seems to be expected but it's marked as readOnly: true.
I'll bring this up upstream
type: string | ||
x-dcl-go-name: Name | ||
x-kubernetes-immutable: true | ||
readOnly: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you copy this directly from the DCL source? It looks like this doesn't exist anymore: https://github.com/GoogleCloudPlatform/declarative-resource-client-library/blob/main/services/google/apikeys/key.yaml#L55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this by hand. I found that yaml file later while I was poking around the client internals. I tried that yaml file too and it returns the same error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah those files should be what we use to generate the TF resource. Ideally we will link directly to those, but I think that has been harder than expected.
From what I can see it seems like the DCL resource expects name
to be defined during creation, but I'm confused because it says output only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a bad assumption as some APIs won't have name field available until you do the 1st GET call after successful creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some recent APIs I worked on:
https://cloud.google.com/storage-transfer/docs/reference/rest/v1/transferJobs/create (Perfect example as the name field is a random serverside generated value)
https://cloud.google.com/api-gateway/docs/reference/rest/v1beta/projects.locations.apis/create (Another example but the expected value that will be returned from the server is predictable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm familiar with the server generated name field. It looks like this API uses the keyId
specified in the create request as the ending of the name field, so it should be possible to remote readOnly: true
on name
and specify it during a test.
The DCL normal flow for creating a resource is to check for existence before creating, which is failing because name is empty.
There is certainly something wrong with the OpenAPI spec in this case, as name should probably be marked required or renamed to keyId or something similar, and I'll get that figured out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, let me know when there is a fix available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have a fix soon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any update on this?
Fixed in #5637 |
Fixes: hashicorp/terraform-provider-google#8959
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)