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 List()
to GPG Keys, ProviderBinaryUploaded
to registry platforms
#602
Conversation
7ff2e3a
to
22885be
Compare
Thanks for adding the changes to the failing tests in this PR. This test
which is fine because I think those two tests within TestRegistryProviderPlatformsList/with_platforms wants to reuse the same provider version. However, the issue is the create api request for the registry platform expects unique os and arch (for the same provider). So I think you may want to overwrite (over the loop on that test) the default values for os and arch (that are set in the helper_test file) so they are unique combination of os and arch.
|
22885be
to
6f082f4
Compare
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.
Not necessarily blocking feedback, but wondering what you think
gpg_key.go
Outdated
@@ -15,6 +15,9 @@ var _ GPGKeys = (*gpgKeys)(nil) | |||
// | |||
// TFE API Docs: https://www.terraform.io/cloud-docs/api-docs/private-registry/gpg-keys | |||
type GPGKeys interface { | |||
// Lists GPG keys in a private registry. | |||
List(ctx context.Context, registryName RegistryName, options *GPGKeyListOptions) (*GPGKeyList, 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.
If filter[namespace] is required, we should not make this a pointer in order to reinforce that, I think
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 was iffy on this and decided on consistency (our ListOpts are almost always pointers). But yeah, I can imagine this confusing someone so I'll change.
gpg_key.go
Outdated
if registryName != PrivateRegistry { | ||
return nil, ErrInvalidRegistryName | ||
} |
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 don't find this very user friendly since it must always be "private". Another alternative is to omit the parameter and allow extension in the future by naming the method "ListPrivate". WDYT?
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 with removing this param and renaming to ListPrivate (on the off chance we end up supporting listing keys from the public registry).
3316e06
to
8f20774
Compare
I merged another PR with just the test fix commit |
8f20774
to
b96a12f
Compare
b96a12f
to
199d4b1
Compare
} | ||
|
||
for _, namespace := range o.Namespaces { | ||
if namespace == "" || !validString(&namespace) { |
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.
validString already checks for nil or empty string
Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes. |
Description
These are two feature requests submitted by @straubt1 (thank you!):
provider-binary-upload
field to response ([Feature Request] Add "provider-binary-uploaded" to response #470)Effectively, this PR adds
List()
to theGPGKeys
interface andProviderBinaryUploaded
field toRegistryPlatform
struct.Note: There does seem to be an issue in our tests with requesting GPG keys for multiple namespaces. The docs specify a comma separated list -- which go-tfe will automagically encode any
[]string
to a comma-separated string if the param starts withfilter[
. I have skipped the test for now.Testing plan