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

Add List() to GPG Keys, ProviderBinaryUploaded to registry platforms #602

Merged
merged 3 commits into from Dec 8, 2022

Conversation

sebasslash
Copy link
Contributor

Description

These are two feature requests submitted by @straubt1 (thank you!):

Effectively, this PR adds List() to the GPGKeys interface and ProviderBinaryUploaded field to RegistryPlatform 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 with filter[. I have skipped the test for now.

Testing plan

go test -v ./... -run TestGPGKeysList
go test -v ./... -run TestRegistryProviderPlatformsList
go test -v ./... -run TestRegistryProviderPlatformsRead

@uturunku1
Copy link
Collaborator

uturunku1 commented Dec 7, 2022

Thanks for adding the changes to the failing tests in this PR. This test TestRegistryProviderPlatformsList/with_platforms is failing because it is iterating over the same versionID

for i := 0; i < numToCreate; i++ {

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.

Copy link
Collaborator

@brandonc brandonc left a 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)
Copy link
Collaborator

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

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 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
Comment on lines 87 to 89
if registryName != PrivateRegistry {
return nil, ErrInvalidRegistryName
}
Copy link
Collaborator

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?

Copy link
Contributor Author

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).

@sebasslash sebasslash force-pushed the super-ticket branch 2 times, most recently from 3316e06 to 8f20774 Compare December 7, 2022 20:34
@brandonc
Copy link
Collaborator

brandonc commented Dec 8, 2022

I merged another PR with just the test fix commit

}

for _, namespace := range o.Namespaces {
if namespace == "" || !validString(&namespace) {
Copy link
Collaborator

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

@sebasslash sebasslash merged commit 21e8cad into main Dec 8, 2022
@sebasslash sebasslash deleted the super-ticket branch December 8, 2022 22:03
@github-actions
Copy link

github-actions bot commented Dec 8, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants