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

Adding pagination for installation, parameter, and credential list result using skip and limit option #2137

Merged
Merged
7 changes: 6 additions & 1 deletion cmd/porter/credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ The results may also be filtered by associated labels and the namespace in which
porter credentials list --namespace prod
porter credentials list --all-namespaces,
porter credentials list --name myapp
porter credentials list --label env=dev`,
porter credentials list --label env=dev
porter credentials list --skip 2 --limit 2`,
PreRunE: func(cmd *cobra.Command, args []string) error {
return opts.Validate()
},
Expand All @@ -163,6 +164,10 @@ The results may also be filtered by associated labels and the namespace in which
"Filter the credential sets by a label formatted as: KEY=VALUE. May be specified multiple times.")
f.StringVarP(&opts.RawFormat, "output", "o", "plaintext",
"Specify an output format. Allowed values: plaintext, json, yaml")
f.Int64Var(&opts.Skip, "skip", 0,
"Skip the number of credential sets by a certain amount. Defaults to 0.")
f.Int64Var(&opts.Limit, "limit", 0,
"Limit the number of credential sets by a certain amount. Defaults to 0.")

return cmd
}
Expand Down
7 changes: 6 additions & 1 deletion cmd/porter/parameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ The results may also be filtered by associated labels and the namespace in which
porter parameters list --namespace prod -o json
porter parameters list --all-namespaces,
porter parameters list --name myapp
porter parameters list --label env=dev`,
porter parameters list --label env=dev
porter parameters list --skip 2 --limit 2`,
PreRunE: func(cmd *cobra.Command, args []string) error {
return opts.Validate()
},
Expand All @@ -162,6 +163,10 @@ The results may also be filtered by associated labels and the namespace in which
"Filter the parameter sets by a label formatted as: KEY=VALUE. May be specified multiple times.")
f.StringVarP(&opts.RawFormat, "output", "o", "plaintext",
"Specify an output format. Allowed values: plaintext, json, yaml")
f.Int64Var(&opts.Skip, "skip", 0,
"Skip the number of parameter sets by a certain amount. Defaults to 0.")
f.Int64Var(&opts.Limit, "limit", 0,
"Limit the number of parameter sets by a certain amount. Defaults to 0.")

return cmd
}
Expand Down
3 changes: 3 additions & 0 deletions docs/content/cli/credentials_list.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ porter credentials list [flags]
porter credentials list --all-namespaces,
porter credentials list --name myapp
porter credentials list --label env=dev
porter credentials list --skip 2 --limit 2
```

### Options
Expand All @@ -34,9 +35,11 @@ porter credentials list [flags]
--all-namespaces Include all namespaces in the results.
-h, --help help for list
-l, --label strings Filter the credential sets by a label formatted as: KEY=VALUE. May be specified multiple times.
--limit int Limit the number of credential sets by a certain amount. Defaults to 0.
--name string Filter the credential sets where the name contains the specified substring.
-n, --namespace string Namespace in which the credential set is defined. Defaults to the global namespace. Use * to list across all namespaces.
-o, --output string Specify an output format. Allowed values: plaintext, json, yaml (default "plaintext")
--skip int Skip the number of credential sets by a certain amount. Defaults to 0.
```

### Options inherited from parent commands
Expand Down
3 changes: 3 additions & 0 deletions docs/content/cli/parameters_list.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ porter parameters list [flags]
porter parameters list --all-namespaces,
porter parameters list --name myapp
porter parameters list --label env=dev
porter parameters list --skip 2 --limit 2
```

### Options
Expand All @@ -34,9 +35,11 @@ porter parameters list [flags]
--all-namespaces Include all namespaces in the results.
-h, --help help for list
-l, --label strings Filter the parameter sets by a label formatted as: KEY=VALUE. May be specified multiple times.
--limit int Limit the number of parameter sets by a certain amount. Defaults to 0.
--name string Filter the parameter sets where the name contains the specified substring.
-n, --namespace string Namespace in which the parameter set is defined. Defaults to the global namespace. Use * to list across all namespaces.
-o, --output string Specify an output format. Allowed values: plaintext, json, yaml (default "plaintext")
--skip int Skip the number of parameter sets by a certain amount. Defaults to 0.
```

### Options inherited from parent commands
Expand Down
8 changes: 7 additions & 1 deletion pkg/porter/credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,13 @@ type CredentialEditOptions struct {

// ListCredentials lists saved credential sets.
func (p *Porter) ListCredentials(ctx context.Context, opts ListOptions) ([]storage.CredentialSet, error) {
return p.Credentials.ListCredentialSets(ctx, opts.GetNamespace(), opts.Name, opts.ParseLabels())
return p.Credentials.ListCredentialSets(ctx, storage.ListOptions{
Namespace: opts.GetNamespace(),
Name: opts.Name,
Labels: opts.ParseLabels(),
Skip: opts.Skip,
Limit: opts.Limit,
})
}

// PrintCredentials prints saved credential sets.
Expand Down
8 changes: 7 additions & 1 deletion pkg/porter/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,13 @@ func (p *Porter) ListInstallations(ctx context.Context, opts ListOptions) (Displ
ctx, log := tracing.StartSpan(ctx)
defer log.EndSpan()

installations, err := p.Installations.ListInstallations(ctx, opts.GetNamespace(), opts.Name, opts.ParseLabels(), opts.Skip, opts.Limit)
installations, err := p.Installations.ListInstallations(ctx, storage.ListOptions{
Namespace: opts.GetNamespace(),
Name: opts.Name,
Labels: opts.ParseLabels(),
Skip: opts.Skip,
Limit: opts.Limit,
})
if err != nil {
return nil, log.Error(fmt.Errorf("could not list installations: %w", err))
}
Expand Down
8 changes: 7 additions & 1 deletion pkg/porter/parameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,13 @@ type ParameterEditOptions struct {

// ListParameters lists saved parameter sets.
func (p *Porter) ListParameters(ctx context.Context, opts ListOptions) ([]storage.ParameterSet, error) {
return p.Parameters.ListParameterSets(ctx, opts.GetNamespace(), opts.Name, opts.ParseLabels())
return p.Parameters.ListParameterSets(ctx, storage.ListOptions{
Namespace: opts.GetNamespace(),
Name: opts.Name,
Labels: opts.ParseLabels(),
Skip: opts.Skip,
Limit: opts.Limit,
})
}

// PrintParameters prints saved parameter sets.
Expand Down
7 changes: 5 additions & 2 deletions pkg/storage/credential_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,13 @@ func (s CredentialStore) InsertCredentialSet(ctx context.Context, creds Credenti
return s.Documents.Insert(ctx, CollectionCredentials, opts)
}

func (s CredentialStore) ListCredentialSets(ctx context.Context, namespace string, name string, labels map[string]string) ([]CredentialSet, error) {
func (s CredentialStore) ListCredentialSets(ctx context.Context, listOptions ListOptions) ([]CredentialSet, error) {
var out []CredentialSet
opts := FindOptions{
Filter: CreateListFiler(namespace, name, labels),
Sort: []string{"namespace", "name"},
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the sort!

Filter: CreateListFiler(listOptions),
Skip: listOptions.Skip,
Limit: listOptions.Limit,
}
err := s.Documents.Find(ctx, CollectionCredentials, opts, &out)
return out, err
Expand Down
61 changes: 56 additions & 5 deletions pkg/storage/credential_store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,35 @@ func TestCredentialStorage_CRUD(t *testing.T) {

require.NoError(t, cp.InsertCredentialSet(context.Background(), cs))

creds, err := cp.ListCredentialSets(context.Background(), "dev", "", nil)
creds, err := cp.ListCredentialSets(context.Background(), ListOptions{
Namespace: "dev",
Name: "",
Labels: nil,
Skip: 0,
Limit: 0,
})
Copy link
Member

Choose a reason for hiding this comment

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

So the nice thing about using an options struct is that you can now omit fields where you aren't changing the default. This allows us to set a better default in the future, and limit how often code has to change when it's about fields that it doesn't care about.

Let's remove the labels, skip and limit fields from here and anywhere else that we aren't setting a non-default value.

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 am also actually not sure why did I do that either... I think my brain automatically did that since seeing the previous function parameter input that lists out all of the default primitive values so I did that to the struct as well 😂 Fixed!

require.NoError(t, err)
require.Len(t, creds, 1, "expected 1 credential set")
require.Equal(t, cs.Name, creds[0].Name, "expected to retrieve secreks credentials")
require.Equal(t, cs.Namespace, creds[0].Namespace, "expected to retrieve secreks credentials")
require.Equal(t, cs.Name, creds[0].Name, "expected to retrieve sekrets credentials")
require.Equal(t, cs.Namespace, creds[0].Namespace, "expected to retrieve sekrets credentials")

creds, err = cp.ListCredentialSets(context.Background(), "", "", nil)
creds, err = cp.ListCredentialSets(context.Background(), ListOptions{
Namespace: "",
Name: "",
Labels: nil,
Skip: 0,
Limit: 0,
})
require.NoError(t, err)
require.Len(t, creds, 0, "expected no global credential sets")

creds, err = cp.ListCredentialSets(context.Background(), "*", "", nil)
creds, err = cp.ListCredentialSets(context.Background(), ListOptions{
Namespace: "*",
Copy link
Member

Choose a reason for hiding this comment

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

So in this case you can remove all of the fields except namespace.

Name: "",
Labels: nil,
Skip: 0,
Limit: 0,
})
require.NoError(t, err)
require.Len(t, creds, 1, "expected 1 credential set defined in all namespaces")

Expand All @@ -45,9 +63,42 @@ func TestCredentialStorage_CRUD(t *testing.T) {
require.NoError(t, err)
assert.Len(t, cs.Credentials, 2)

cs2 := NewCredentialSet("dev", "sekrets-2", secrets.Strategy{
Name: "password-2", Source: secrets.Source{
Key: "secret-2",
Value: "dbPassword-2"}})
require.NoError(t, cp.InsertCredentialSet(context.Background(), cs2))

creds, err = cp.ListCredentialSets(context.Background(), ListOptions{
Namespace: "dev",
Name: "",
Labels: nil,
Skip: 1,
Limit: 0,
})
require.NoError(t, err)
require.Len(t, creds, 1, "expected 1 credential set")
require.Equal(t, cs2.Name, creds[0].Name, "expected to retrieve sekrets-2 credentials")
require.Equal(t, cs2.Namespace, creds[0].Namespace, "expected to retrieve sekrets-2 credentials")

creds, err = cp.ListCredentialSets(context.Background(), ListOptions{
Namespace: "dev",
Name: "",
Labels: nil,
Skip: 0,
Limit: 1,
})
require.NoError(t, err)
require.Len(t, creds, 1, "expected 1 credential set")
require.Equal(t, cs.Name, creds[0].Name, "expected to retrieve sekrets credentials")
require.Equal(t, cs.Namespace, creds[0].Namespace, "expected to retrieve sekrets credentials")

require.NoError(t, cp.RemoveCredentialSet(context.Background(), cs.Namespace, cs.Name))
require.NoError(t, cp.RemoveCredentialSet(context.Background(), cs2.Namespace, cs2.Name))
_, err = cp.GetCredentialSet(context.Background(), cs.Namespace, cs.Name)
require.ErrorIs(t, err, ErrNotFound{})
_, err = cp.GetCredentialSet(context.Background(), cs2.Namespace, cs2.Name)
require.ErrorIs(t, err, ErrNotFound{})
}

func TestCredentialStorage_Validate_GoodSources(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/storage/credentialset_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ type CredentialSetProvider interface {
ResolveAll(ctx context.Context, creds CredentialSet) (secrets.Set, error)
Validate(ctx context.Context, creds CredentialSet) error
InsertCredentialSet(ctx context.Context, creds CredentialSet) error
ListCredentialSets(ctx context.Context, namespace string, name string, labels map[string]string) ([]CredentialSet, error)
ListCredentialSets(ctx context.Context, listOptions ListOptions) ([]CredentialSet, error)
GetCredentialSet(ctx context.Context, namespace string, name string) (CredentialSet, error)
UpdateCredentialSet(ctx context.Context, creds CredentialSet) error
RemoveCredentialSet(ctx context.Context, namespace string, name string) error
Expand Down
2 changes: 1 addition & 1 deletion pkg/storage/installation_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type InstallationProvider interface {
GetInstallation(ctx context.Context, namespace string, name string) (Installation, error)

// ListInstallations returns Installations sorted in ascending order by the namespace and then name.
ListInstallations(ctx context.Context, namespace string, name string, labels map[string]string, skip int64, limit int64) ([]Installation, error)
ListInstallations(ctx context.Context, listOption ListOptions) ([]Installation, error)

// ListRuns returns Run documents sorted in ascending order by ID.
ListRuns(ctx context.Context, namespace string, installation string) ([]Run, map[string][]Result, error)
Expand Down
8 changes: 4 additions & 4 deletions pkg/storage/installation_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,16 @@ func (s InstallationStore) Initialize(ctx context.Context) error {
return span.Error(err)
}

func (s InstallationStore) ListInstallations(ctx context.Context, namespace string, name string, labels map[string]string, skip int64, limit int64) ([]Installation, error) {
func (s InstallationStore) ListInstallations(ctx context.Context, listOptions ListOptions) ([]Installation, error) {
_, log := tracing.StartSpan(ctx)
defer log.EndSpan()

var out []Installation
findOpts := FindOptions{
Sort: []string{"namespace", "name"},
Filter: CreateListFiler(namespace, name, labels),
Skip: skip,
Limit: limit,
Filter: CreateListFiler(listOptions),
Skip: listOptions.Skip,
Limit: listOptions.Limit,
}

err := s.store.Find(ctx, CollectionInstallations, findOpts, &out)
Expand Down
40 changes: 35 additions & 5 deletions pkg/storage/installation_store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,13 @@ func TestInstallationStorageProvider_Installations(t *testing.T) {
defer cp.Close()

t.Run("ListInstallations", func(t *testing.T) {
installations, err := cp.ListInstallations(context.Background(), "dev", "", nil, 0, 0)
installations, err := cp.ListInstallations(context.Background(), ListOptions{
Namespace: "dev",
Name: "",
Labels: nil,
Skip: 0,
Limit: 0,
})
require.NoError(t, err, "ListInstallations failed")

require.Len(t, installations, 3, "Expected 3 installations")
Expand All @@ -184,7 +190,13 @@ func TestInstallationStorageProvider_Installations(t *testing.T) {
})

t.Run("ListInstallations with skip option", func(t *testing.T) {
installations, err := cp.ListInstallations(context.Background(), "dev", "", nil, 1, 0)
installations, err := cp.ListInstallations(context.Background(), ListOptions{
Namespace: "dev",
Name: "",
Labels: nil,
Skip: 1,
Limit: 0,
})
require.NoError(t, err, "ListInstallations failed")

require.Len(t, installations, 2, "Expected 2 installations")
Expand All @@ -199,7 +211,13 @@ func TestInstallationStorageProvider_Installations(t *testing.T) {
})

t.Run("ListInstallations with limit option", func(t *testing.T) {
installations, err := cp.ListInstallations(context.Background(), "dev", "", nil, 0, 2)
installations, err := cp.ListInstallations(context.Background(), ListOptions{
Namespace: "dev",
Name: "",
Labels: nil,
Skip: 0,
Limit: 2,
})
require.NoError(t, err, "ListInstallations failed")

require.Len(t, installations, 2, "Expected 2 installations")
Expand Down Expand Up @@ -266,14 +284,26 @@ func TestInstallationStorageProvider_DeleteInstallation(t *testing.T) {
cp := generateInstallationData(t)
defer cp.Close()

installations, err := cp.ListInstallations(context.Background(), "dev", "", nil, 0, 0)
installations, err := cp.ListInstallations(context.Background(), ListOptions{
Namespace: "dev",
Name: "",
Labels: nil,
Skip: 0,
Limit: 0,
})
require.NoError(t, err, "ListInstallations failed")
assert.Len(t, installations, 3, "expected 3 installations")

err = cp.RemoveInstallation(context.Background(), "dev", "foo")
require.NoError(t, err, "RemoveInstallation failed")

installations, err = cp.ListInstallations(context.Background(), "dev", "", nil, 0, 0)
installations, err = cp.ListInstallations(context.Background(), ListOptions{
Namespace: "dev",
Name: "",
Labels: nil,
Skip: 0,
Limit: 0,
})
require.NoError(t, err, "ListInstallations failed")
assert.Len(t, installations, 2, "expected foo to be deleted")

Expand Down
Loading