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 registry provider client and integration tests #404

Closed
wants to merge 4 commits into from

Conversation

annawinkler
Copy link
Contributor

@annawinkler annawinkler commented May 17, 2022

Description

This PR was broken out of this PR.

This PR adds support for the private registry provider API.

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, 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

...

Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

Great work @annawinkler and @joekarl ... I have not been able to review all of it as it is quite a hefty PR, but hopefully a lot the comments translate to the rest of the unreviewed code.

Files I've reviewed:
helper_test.go
registry_provider.go
And the List test in the registry_provider_integration_test.go

Definitely would like @brandonc and @uturunku1 to give it a pass.

Name string `jsonapi:"attr,name"`
RegistryName RegistryName `jsonapi:"attr,registry-name"`
Permissions *RegistryProviderPermissions `jsonapi:"attr,permissions"`
CreatedAt string `jsonapi:"attr,created-at"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be jsonapi:"attr,created-at,iso8601"

RegistryName RegistryName `jsonapi:"attr,registry-name"`
Permissions *RegistryProviderPermissions `jsonapi:"attr,permissions"`
CreatedAt string `jsonapi:"attr,created-at"`
UpdatedAt string `jsonapi:"attr,updated-at"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be jsonapi:"attr,updated-at,iso8601"


type RegistryProviderListOptions struct {
ListOptions
// A query string to filter by registry_name
Copy link
Contributor

Choose a reason for hiding this comment

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

We follow the doc convention: // Optional: A query string to filter by registry_name even though the field being a pointer is implying that it's optional.

Items []*RegistryProvider
}

func (o RegistryProviderListOptions) valid() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

valid() methods go all the way in the bottom of the file ⏬
For context, file should be organized in this order:

  1. Interface/struct definitions
  2. Endpoint methods
  3. Valid and other helper methods

}

func (r *registryProviders) List(ctx context.Context, organization string, options *RegistryProviderListOptions) (*RegistryProviderList, error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary whitespace ✏️

}

func (r *registryProviders) Read(ctx context.Context, organization string, registryName RegistryName, namespace string, name string, options *RegistryProviderReadOptions) (*RegistryProvider, error) {
if !validStringID(&organization) {
Copy link
Contributor

Choose a reason for hiding this comment

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

All these validStringID() checks should probably be refactored into a valid() function and perhaps should live in the ReadOptionsStruct instead of as method params (back to my previous point). 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.

This has been refactored.

providerTest, _ := createPublicRegistryProvider(t, client, orgTest)
providers = append(providers, providerTest)
}
for i := 0; i < createN; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it but we create private and public registry providers in the test.

assert.NotEmpty(t, returnedProviders.Items)
assert.Equal(t, providerN, returnedProviders.TotalCount)
assert.Equal(t, 1, returnedProviders.TotalPages)
for _, rp := range returnedProviders.Items {
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we can condense this logic to:

for _, p := range providers {
  assert.Contains(t, returnedProviders.Items, p)
}

Careful with assert.Contains() as it performs deep equality. (I've been burned before with Included relationships before 👉 🔥 )

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore, this sort of logic (checking that the desired provider(s) exists) seems better reserved for the Create test; for this test we should only care about that endpoint returns the correct # of elements, query params work as intended and that the fields are deserialized properly (although we can reserve checking the deserialization of fields in the Read test).

}
})

t.Run("returns pages", 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.

For this kind of test, the usual convention is to set PageSize and PageNumber to some incredibly large number, which I think is the easiest way to verify pagination is working as intended. Here's an example:

t.Run("with list options", func(t *testing.T) {
// Request a page number which is out of range. The result should
// be successful, but return no results if the paging options are
// properly passed along.
wl, err := client.Workspaces.List(ctx, orgTest.Name, &WorkspaceListOptions{
ListOptions: ListOptions{
PageNumber: 999,
PageSize: 100,
},
})
require.NoError(t, err)
assert.Empty(t, wl.Items)
assert.Equal(t, 999, wl.CurrentPage)
assert.Equal(t, 2, wl.TotalCount)
})

expectedProvider := providers[0]
returnedProviders, err := client.RegistryProviders.List(ctx, orgTest.Name, &RegistryProviderListOptions{
Search: &expectedProvider.Name,
ListOptions: ListOptions{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we omit the ListOptions field here?

@annawinkler annawinkler force-pushed the aw/add-private-registry-provider branch from 35f00e1 to a61cf7f Compare May 23, 2022 19:12
@annawinkler annawinkler force-pushed the aw/add-private-registry-provider branch from a61cf7f to 5cb09b9 Compare May 23, 2022 19:14
@annawinkler
Copy link
Contributor Author

Closing this PR in favor of the one large one since refactoring has occurred there. I'll address the review comments in PR 313.

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