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 support for GPG Keys API #429

Merged
merged 1 commit into from Jun 29, 2022
Merged

Add support for GPG Keys API #429

merged 1 commit into from Jun 29, 2022

Conversation

sebasslash
Copy link
Contributor

@sebasslash sebasslash commented Jun 13, 2022

Adds support for the GPG Keys API

This PR also modifies the client to support the registry base path: /api/registry. To keep the client's internal interface consistent I've added an extra check in newRequest() that will parse the path variable; if a request is made to the Private Registry API the url must explicitly contain the base path so that the client can set the appropriate registry base path. As far as the TFC/E API is concerned, the GPG Key endpoints are the only ones that use the registry's default path.

To run individual tests:

go test -run TestGPGKeyCreate -v ./... -tags=integration
go test -run TestGPGKeyRead -v ./... -tags=integration
go test -run TestGPGKeyUpdate -v ./... -tags=integration
go test -run TestGPGKeyDelete -v ./... -tags=integration

gpg_key.go Outdated Show resolved Hide resolved
gpg_key.go Outdated Show resolved Hide resolved
gpg_key.go Outdated
return nil, err
}

req, err := s.client.newRequest("POST", "/api/registry/private/v2/gpg-keys", &options)
Copy link
Contributor

Choose a reason for hiding this comment

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

When we talked about these URLs last week, did we say that we would create a new request type for the private registry API endpoint? 🤔 I didn't take notes so I'm not remembering unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! but these improvements will be made after the fact since they are only internal changes.

Copy link
Contributor

@annawinkler annawinkler left a comment

Choose a reason for hiding this comment

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

Thanks for adding support for this API! I've left a few comments and questions. ✨

gpg_key.go Outdated Show resolved Hide resolved
assert.Equal(t, gpgKey.KeyID, updatedKey.KeyID)
assert.Equal(t, updatedKey.Namespace, provider2.Organization.Name)

// Cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 Nice!


gpgKey, _ := createGPGKey(t, client, org, provider)

t.Run("when a key exists", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a test if the key doesn't exist or is invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will return a 404, however we don't need to add a test for this. The reason being is the client in client.do() decodes the headers and returns the appropriate generic error. In this case that would be ErrResourceNotFound.

}

u := fmt.Sprintf("/api/registry/private/v2/gpg-keys/%s/%s",
url.QueryEscape(keyID.Namespace),
Copy link
Contributor

Choose a reason for hiding this comment

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

When I was reading the docs, I noticed this line: Only the namespace attribute can be updated, and namespace has to match an organization the user has permission to access. I'm curious if the API returns an error if a namespace doesn't match an org the user has permission to access? Would this be something we would want to add a test for? The reason I suggest adding a test is because this detail could be overlooked by someone using this API, and they might see a test. I also admit that we don't want to cross a boundary if it doesn't make sense. What do you 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.

Good catch and this makes way more sense here as we want to test for an error that's specific to this resource! I've added a check in the error handler for client.do() inside GPGKeys.Update() where if the body's message contains namespace is not authorized it will return ErrNamespaceNotAuthorized. I've also added the corresponding test.

@sebasslash sebasslash force-pushed the sebasslash/gpg-key-api branch 2 times, most recently from 9c7f6d2 to 82712d4 Compare June 13, 2022 21:26
gpg_key.go Outdated
return nil, err
}

u := fmt.Sprintf("/api/registry/private/v2/gpg-keys/%s/%s",
Copy link
Contributor

Choose a reason for hiding this comment

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

I have mixed thoughts on not taking a parameter for the registry-name (which must be private). The API docs specify this as a parameter, and this is a parameter for the private registry provider APIs. What are your thoughts about this @sebasslash ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, well since it's always going to be private, I'd rather not expose it as part of the interface.

Copy link
Contributor

@annawinkler annawinkler left a comment

Choose a reason for hiding this comment

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

🏅 This is looking really good! I have one more question that relates to the private registry provider APIs and how we want to deal with a parameter (in this case registry-name) that can only have one value.

@sebasslash sebasslash force-pushed the sebasslash/gpg-key-api branch 4 times, most recently from 07c1e52 to 9be5c23 Compare June 27, 2022 17:19
Copy link
Contributor

@annawinkler annawinkler left a comment

Choose a reason for hiding this comment

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

🥇 Looks great!

@sebasslash sebasslash merged commit 767dc58 into main Jun 29, 2022
@sebasslash sebasslash deleted the sebasslash/gpg-key-api branch June 29, 2022 15:49
@github-actions
Copy link

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

2 participants