✨ Add private registry provider version #405

annawinkler · 2022-05-17T23:24:55Z

Description

This PR was broken out of this PR.

This PR adds support for the private registry provider version API. The first PR for these set of changes is this one.

Testing plan

To test these changes locally, create a local go project, and in the go.mod include a line similar to the following:

replace github.com/hashicorp/go-tfe => /Users/YOURUSER/go/src/github.com/hashicorp/go-tfe

Then, for a registry provider version, you can test:

Create
Delete
Read
List

External links

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

$ TFE_ADDRESS="https://example" TFE_TOKEN="example" TF_ACC="1" go test ./... -v -tags=integration -run TestFunctionsAffectedByChange

...

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1202295552965199

brandonc

(As I mentioned in slack, I only got about halfway through this review)

brandonc · 2022-05-18T20:49:12Z

errors.go

+
+	ErrInvalidRegistryName = errors.New("invalid value for registry-name")
+
+	ErrInvalidRegistryNameType = errors.New("invalid type for registry-name. Please use 'RegistryName'")


I don't think this error is used (And if I understand the point of the error, the value should be "registry-providers")

brandonc · 2022-05-18T20:51:44Z

helper_test.go

+	prv, err := client.RegistryProviders.Create(ctx, org.Name, options)
+
+	if err != nil {
+		t.Fatal(err)


I think we should return something here, since prv may be nil and the next line will panic

I think the line does we want it to - Fatal is equivalent to Log plus FailNow. FailNow stops execution, calls runtime.Goexit, and continues execution with the next test or benchmark.

brandonc · 2022-05-18T20:53:46Z

helper_test.go

+	}
+}
+
+func createPublicRegistryProvider(t *testing.T, client *Client, org *Organization) (*RegistryProvider, func()) {


💅 this and the previous method have so much in common, I'm wondering if there should be a third method that parameterizes PublicRegistry/PrivateRegistry ?

brandonc · 2022-05-18T21:00:49Z

helper_test.go

+	prvv, err := client.RegistryProviderVersions.Create(ctx, providerID, options)
+
+	if err != nil {
+		t.Fatal(err)


same 'return' concern as above

brandonc · 2022-05-18T21:04:34Z

registry_provider.go

+
+// List of available includes
+const (
+	RegistryProviderVersionsInclude RegistryProviderIncludeOps = "registry-provider-versions"


I don't see this documented or in the API. Is it possible to include this relationship?

🤔 You're right, when I re-reviewed the documentation, it is not clear that this include is an option. I tested it locally with this request:

curl --location --request GET 'https://LOCALURL/api/v2/organizations/amazing-org/registry-providers/private/amazing-org/zebra-provider?include=registry-provider-versions' \ --header 'Content-Type: application/vnd.api+json' \ --header 'Authorization: Bearer TOKEN' \ --header 'Cookie: __profilin=p%3Dt' \ --data-raw '{ "data": { "type": "registry-providers", "attributes": { "name": "test-registry-provider-5de6cb61-b5ea-e72e-13a2-c9aab0b5c3ac", "namespace": "amazing-org", "registry-name": "private" } } }'

And the response does include the registry provider versions (see the attached json). I think we'll need to update the docs. What are your thoughts?

response.txt

brandonc

Looks really close! It would be a good time to update the README Coverage and CHANGELOG with the new endpoints

brandonc · 2022-05-18T22:14:34Z

registry_provider.go

+
+type RegistryProviderPermissions struct {
+	CanDelete bool `jsonapi:"attr,can-delete,omitempty"`
+	CanUploadAsset bool `jsonapi:"attr,can-upload-asset,omitempty"`


I don't see this field in the documentation or API serializer. I think it's only in the RegistryProviderVersions API

brandonc · 2022-05-18T22:16:49Z

registry_provider.go

+type RegistryProviderPermissions struct {
+	CanDelete bool `jsonapi:"attr,can-delete,omitempty"`
+	CanUploadAsset bool `jsonapi:"attr,can-upload-asset,omitempty"`


These fields are marked omitempty but it's not possible for this type to be empty. Looking at the provider API, it looks like these permissions fields are always present anyway, so the Permissions field probably doesn't need to be a pointer.

brandonc · 2022-05-18T22:23:53Z

registry_provider.go

+}
+
+// RegistryProviderID is the multi key ID for addressing a provider
+type RegistryProviderID struct {


I don't think this needs jsonapi metadata since it is only used as a compound key and not serialized into a jsonapi request body.

brandonc · 2022-05-18T22:26:59Z

registry_provider.go

+	if options.RegistryName == PrivateRegistry && organization != options.Namespace {
+		return nil, ErrInvalidPrivateProviderNamespaceDoesntMatchOrganization
+	}


:thinkies: I wonder if we should just let the API handle this error since it feels like it crosses over into API design & behavior?

brandonc · 2022-05-18T22:29:07Z

registry_provider.go

+	if err := id.RegistryName.valid(); err != nil {
+		return err
+	}
+	return nil


💅 this is the same as return id.RegistryName.valid() as the last statement

brandonc · 2022-05-18T22:35:33Z

registry_provider_version.go

+
+// RegistryProviderVersionID is the multi key ID for addressing a version provider
+type RegistryProviderVersionID struct {
+	RegistryProviderID


Cool use of embedded struct

brandonc · 2022-05-18T22:35:45Z

registry_provider_version.go

+// RegistryProviderVersionID is the multi key ID for addressing a version provider
+type RegistryProviderVersionID struct {
+	RegistryProviderID
+	Version string `jsonapi:"attr,version"`


I don't think this needs jsonapi metadata.

brandonc · 2022-05-18T22:39:55Z

registry_provider_version.go

+type RegistryProviderVersionCreateOptions struct {
+	Version   string   `jsonapi:"attr,version"`
+	KeyID     string   `jsonapi:"attr,key-id"`
+	Protocols []string `jsonapi:"attr,protocols"`


I wonder if we should strongly type these versions: ProviderAPI4, ProviderAPI5, ProviderAPI6 ? Might reduce confusion, but then we would have to maintain it. It looks like the API is strict about what is accepted here.

I like that idea!

brandonc · 2022-05-18T22:43:04Z

registry_provider_version.go

+	return r.client.do(ctx, req, nil)
+}
+
+func (v RegistryProviderVersion) ShasumsUploadURL() (string, error) {


Looking across the repo, it looks like we've been really relaxed about wrapping Links with go functions. But I like it!

annawinkler · 2022-05-23T20:51:56Z

I'm going to close this PR in favor of the one large one. I'll address the comments here in the one big PR.

Karl Kirch added 2 commits May 17, 2022 16:55

Add registry provider client and integration tests

35f00e1

Add registry provider version

52b33a2

brandonc reviewed May 18, 2022

View reviewed changes

annawinkler closed this May 23, 2022

annawinkler deleted the aw/add-registry-provider-version branch June 8, 2022 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add private registry provider version #405

✨ Add private registry provider version #405

annawinkler commented May 17, 2022 •

edited

Loading

brandonc left a comment

brandonc May 18, 2022 •

edited

Loading

brandonc May 18, 2022

annawinkler May 23, 2022

brandonc May 18, 2022

brandonc May 18, 2022

brandonc May 18, 2022

annawinkler May 23, 2022

brandonc left a comment

brandonc May 18, 2022

brandonc May 18, 2022

brandonc May 18, 2022

brandonc May 18, 2022

brandonc May 18, 2022

brandonc May 18, 2022

brandonc May 18, 2022

brandonc May 18, 2022

annawinkler May 23, 2022

brandonc May 18, 2022

annawinkler commented May 23, 2022


		ErrInvalidRegistryName = errors.New("invalid value for registry-name")

		ErrInvalidRegistryNameType = errors.New("invalid type for registry-name. Please use 'RegistryName'")

✨ Add private registry provider version #405

✨ Add private registry provider version #405

Conversation

annawinkler commented May 17, 2022 • edited Loading

Description

Testing plan

External links

Output from tests

brandonc left a comment

Choose a reason for hiding this comment

brandonc May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annawinkler commented May 23, 2022

annawinkler commented May 17, 2022 •

edited

Loading

brandonc May 18, 2022 •

edited

Loading