From d7009a21da079d129c57045e43132ab77c816e49 Mon Sep 17 00:00:00 2001 From: Joshua Bezaleel Abednego Date: Thu, 16 Jun 2022 04:20:24 +0700 Subject: [PATCH] Adding pagination for installation, parameter, and credential list result using skip and limit option (#2137) * Add pagination option for installation list command using skip and limit flag Signed-off-by: joshuabezaleel * Increase plugin start/stop timeouts As I was adding back in net/rpc plugins (the legacy v0 plugins), I realized that our plugin timeouts don't work well for net/rpc since it is much slower than gRPC. I've bumped both the plugin start and stop timeout defaults to make it less likely that a user will run into the timeout, while still giving us a good "oops the plugin is broken" timeout detection. Signed-off-by: Carolyn Van Slyck Signed-off-by: joshuabezaleel * Add InstallationStore.FindInstallations (#2119) The advanced dependencies proposal needs to be able to search for installations based on more complex critieria than is available in the ListInstallations function (which is intended to support the porter installation list command). FindInstallations lets us craft any valid mongodb find query and execute it, returning a list of installations. Signed-off-by: Carolyn Van Slyck Signed-off-by: joshuabezaleel * Rename DisplayRun.ClaimID to ID I missed this field when I did a sweep earlier to remove the use of the word claim in the release/v1 branch. In the rest of the CLI's output we call the run's id just ID or RunID, and should be consistent with that. I've changed DisplayID.ClaimID to ID so that we aren't exposing the term claim to our users (and it's not really the claim id anymore anyway). Signed-off-by: Carolyn Van Slyck Signed-off-by: joshuabezaleel * Support Docker TLS environment variables We are using the docker cli library to build images and I had thought this gave us automatic support for building against a remote docker host. It works fine for DOCKER_HOST, but turns out the TLS configuration environment variables are only parsed when the docker CLI flags are bound (which doesn't occur when we use it as a library). I've updated how we initialize the docker cli library so that DOCKER_TLS_VERIFY and DOCKER_CERT_PATH are picked up and passed to the library. Signed-off-by: Carolyn Van Slyck Signed-off-by: joshuabezaleel * Add vet and lint targets to magefile Signed-off-by: Tanmay Chaudhry Signed-off-by: joshuabezaleel * Add ListOption input parameter struct and enable skip and limit option to credential and parameter list command as well Signed-off-by: joshuabezaleel * Leave out default value for ListOption's properties Signed-off-by: joshuabezaleel * Remove commented function signature Signed-off-by: joshuabezaleel * Convert CreateListFilter to ToFindOptions method for ListOptions type receiver Signed-off-by: joshuabezaleel Co-authored-by: Carolyn Van Slyck Co-authored-by: Tanmay Chaudhry --- cmd/porter/credentials.go | 7 +++- cmd/porter/installations.go | 7 +++- cmd/porter/parameters.go | 7 +++- docs/content/cli/build.md | 4 +-- docs/content/cli/bundles_build.md | 4 +-- docs/content/cli/credentials_list.md | 3 ++ docs/content/cli/installations_list.md | 3 ++ docs/content/cli/list.md | 3 ++ docs/content/cli/parameters_list.md | 3 ++ pkg/cli/config.go | 2 ++ pkg/porter/credentials.go | 8 ++++- pkg/porter/list.go | 10 +++++- pkg/porter/parameters.go | 8 ++++- pkg/storage/credential_store.go | 7 ++-- pkg/storage/credential_store_test.go | 31 ++++++++++++++--- pkg/storage/credentialset_provider.go | 2 +- pkg/storage/installation_provider.go | 2 +- pkg/storage/installation_store.go | 9 ++--- pkg/storage/installation_store_test.go | 36 ++++++++++++++++++-- pkg/storage/migrations/manager_test.go | 24 ++++++++----- pkg/storage/parameter_store.go | 7 ++-- pkg/storage/parameter_store_test.go | 41 ++++++++++++++++++---- pkg/storage/parameterset_provider.go | 2 +- pkg/storage/query.go | 47 ++++++++++++++++++++------ pkg/storage/query_test.go | 25 ++++++++++++++ 25 files changed, 238 insertions(+), 64 deletions(-) diff --git a/cmd/porter/credentials.go b/cmd/porter/credentials.go index 31c934c94..30d878411 100644 --- a/cmd/porter/credentials.go +++ b/cmd/porter/credentials.go @@ -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() }, @@ -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 } diff --git a/cmd/porter/installations.go b/cmd/porter/installations.go index b78b16abd..04abb8b4a 100644 --- a/cmd/porter/installations.go +++ b/cmd/porter/installations.go @@ -44,7 +44,8 @@ Optional output formats include json and yaml.`, porter installations list -o json porter installations list --all-namespaces, porter installations list --label owner=myname --namespace dev - porter installations list --name myapp`, + porter installations list --name myapp + porter installations list --skip 2 --limit 2`, PreRunE: func(cmd *cobra.Command, args []string) error { return opts.Validate() }, @@ -64,6 +65,10 @@ Optional output formats include json and yaml.`, "Filter the installations 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 installations by a certain amount. Defaults to 0.") + f.Int64Var(&opts.Limit, "limit", 0, + "Limit the number of installations by a certain amount. Defaults to 0.") return cmd } diff --git a/cmd/porter/parameters.go b/cmd/porter/parameters.go index ef004dd48..ea00b938c 100644 --- a/cmd/porter/parameters.go +++ b/cmd/porter/parameters.go @@ -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() }, @@ -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 } diff --git a/docs/content/cli/build.md b/docs/content/cli/build.md index 83f25cd28..155de2177 100644 --- a/docs/content/cli/build.md +++ b/docs/content/cli/build.md @@ -39,8 +39,8 @@ porter build [flags] ``` --build-arg stringArray Set build arguments in the template Dockerfile (format: NAME=VALUE). May be specified multiple times. --custom stringArray Define an individual key-value pair for the custom section in the form of NAME=VALUE. Use dot notation to specify a nested custom field. May be specified multiple times. - -d, --dir string Path to the build context directory where all bundle assets are located. - -f, --file porter.yaml Path to the Porter manifest. Defaults to porter.yaml in the current directory. + -d, --dir string Path to the build context directory where all bundle assets are located. Defaults to the current directory. + -f, --file string Path to the Porter manifest. The path is relative to the build context directory. Defaults to porter.yaml in the current directory. -h, --help help for build --name string Override the bundle name --no-cache Do not use the Docker cache when building the bundle's invocation image. diff --git a/docs/content/cli/bundles_build.md b/docs/content/cli/bundles_build.md index 9f7a0f1bf..6498ac0df 100644 --- a/docs/content/cli/bundles_build.md +++ b/docs/content/cli/bundles_build.md @@ -39,8 +39,8 @@ porter bundles build [flags] ``` --build-arg stringArray Set build arguments in the template Dockerfile (format: NAME=VALUE). May be specified multiple times. --custom stringArray Define an individual key-value pair for the custom section in the form of NAME=VALUE. Use dot notation to specify a nested custom field. May be specified multiple times. - -d, --dir string Path to the build context directory where all bundle assets are located. - -f, --file porter.yaml Path to the Porter manifest. Defaults to porter.yaml in the current directory. + -d, --dir string Path to the build context directory where all bundle assets are located. Defaults to the current directory. + -f, --file string Path to the Porter manifest. The path is relative to the build context directory. Defaults to porter.yaml in the current directory. -h, --help help for build --name string Override the bundle name --no-cache Do not use the Docker cache when building the bundle's invocation image. diff --git a/docs/content/cli/credentials_list.md b/docs/content/cli/credentials_list.md index 5a5940d53..d0ec4fef5 100644 --- a/docs/content/cli/credentials_list.md +++ b/docs/content/cli/credentials_list.md @@ -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 @@ -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 diff --git a/docs/content/cli/installations_list.md b/docs/content/cli/installations_list.md index 33e68c361..081fdf281 100644 --- a/docs/content/cli/installations_list.md +++ b/docs/content/cli/installations_list.md @@ -29,6 +29,7 @@ porter installations list [flags] porter installations list --all-namespaces, porter installations list --label owner=myname --namespace dev porter installations list --name myapp + porter installations list --skip 2 --limit 2 ``` ### Options @@ -37,9 +38,11 @@ porter installations list [flags] --all-namespaces Include all namespaces in the results. -h, --help help for list -l, --label strings Filter the installations by a label formatted as: KEY=VALUE. May be specified multiple times. + --limit int Limit the number of installations by a certain amount. Defaults to 0. --name string Filter the installations where the name contains the specified substring. -n, --namespace string Filter the installations by namespace. Defaults to the global namespace. -o, --output string Specify an output format. Allowed values: plaintext, json, yaml (default "plaintext") + --skip int Skip the number of installations by a certain amount. Defaults to 0. ``` ### Options inherited from parent commands diff --git a/docs/content/cli/list.md b/docs/content/cli/list.md index b3388da37..d7a44d0cd 100644 --- a/docs/content/cli/list.md +++ b/docs/content/cli/list.md @@ -29,6 +29,7 @@ porter list [flags] porter list --all-namespaces, porter list --label owner=myname --namespace dev porter list --name myapp + porter list --skip 2 --limit 2 ``` ### Options @@ -37,9 +38,11 @@ porter list [flags] --all-namespaces Include all namespaces in the results. -h, --help help for list -l, --label strings Filter the installations by a label formatted as: KEY=VALUE. May be specified multiple times. + --limit int Limit the number of installations by a certain amount. Defaults to 0. --name string Filter the installations where the name contains the specified substring. -n, --namespace string Filter the installations by namespace. Defaults to the global namespace. -o, --output string Specify an output format. Allowed values: plaintext, json, yaml (default "plaintext") + --skip int Skip the number of installations by a certain amount. Defaults to 0. ``` ### Options inherited from parent commands diff --git a/docs/content/cli/parameters_list.md b/docs/content/cli/parameters_list.md index 5787a42c9..a6ade63e6 100644 --- a/docs/content/cli/parameters_list.md +++ b/docs/content/cli/parameters_list.md @@ -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 @@ -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 diff --git a/pkg/cli/config.go b/pkg/cli/config.go index 2f7ca3328..f7ce91413 100644 --- a/pkg/cli/config.go +++ b/pkg/cli/config.go @@ -71,6 +71,8 @@ func getViperValue(flags *pflag.FlagSet, f *pflag.Flag) interface{} { switch flagType { case "int": out, err = flags.GetInt(f.Name) + case "int64": + out, err = flags.GetInt64(f.Name) case "string": out, err = flags.GetString(f.Name) case "bool": diff --git a/pkg/porter/credentials.go b/pkg/porter/credentials.go index f30b1365f..05e83720d 100644 --- a/pkg/porter/credentials.go +++ b/pkg/porter/credentials.go @@ -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. diff --git a/pkg/porter/list.go b/pkg/porter/list.go index a8aeb8765..84b746543 100644 --- a/pkg/porter/list.go +++ b/pkg/porter/list.go @@ -24,6 +24,8 @@ type ListOptions struct { Namespace string Name string Labels []string + Skip int64 + Limit int64 } func (o *ListOptions) Validate() error { @@ -218,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()) + 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)) } diff --git a/pkg/porter/parameters.go b/pkg/porter/parameters.go index 2dc9467db..218944f65 100644 --- a/pkg/porter/parameters.go +++ b/pkg/porter/parameters.go @@ -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. diff --git a/pkg/storage/credential_store.go b/pkg/storage/credential_store.go index 71acd6008..e30ee6d51 100644 --- a/pkg/storage/credential_store.go +++ b/pkg/storage/credential_store.go @@ -110,12 +110,9 @@ 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), - } - err := s.Documents.Find(ctx, CollectionCredentials, opts, &out) + err := s.Documents.Find(ctx, CollectionCredentials, listOptions.ToFindOptions(), &out) return out, err } diff --git a/pkg/storage/credential_store_test.go b/pkg/storage/credential_store_test.go index 9ee62abc0..0336ad917 100644 --- a/pkg/storage/credential_store_test.go +++ b/pkg/storage/credential_store_test.go @@ -20,17 +20,17 @@ 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"}) 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{}) 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: "*"}) require.NoError(t, err) require.Len(t, creds, 1, "expected 1 credential set defined in all namespaces") @@ -45,9 +45,30 @@ 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", Skip: 1}) + 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", 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) { diff --git a/pkg/storage/credentialset_provider.go b/pkg/storage/credentialset_provider.go index 8db1faa07..cfa83eb37 100644 --- a/pkg/storage/credentialset_provider.go +++ b/pkg/storage/credentialset_provider.go @@ -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 diff --git a/pkg/storage/installation_provider.go b/pkg/storage/installation_provider.go index 6c8111c13..9d0b6b929 100644 --- a/pkg/storage/installation_provider.go +++ b/pkg/storage/installation_provider.go @@ -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) ([]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) diff --git a/pkg/storage/installation_store.go b/pkg/storage/installation_store.go index 9dc4cf66e..bfda84df0 100644 --- a/pkg/storage/installation_store.go +++ b/pkg/storage/installation_store.go @@ -63,17 +63,12 @@ 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) ([]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), - } - - err := s.store.Find(ctx, CollectionInstallations, findOpts, &out) + err := s.store.Find(ctx, CollectionInstallations, listOptions.ToFindOptions(), &out) return out, err } diff --git a/pkg/storage/installation_store_test.go b/pkg/storage/installation_store_test.go index 3a0c5f9d6..6c2ecaa34 100644 --- a/pkg/storage/installation_store_test.go +++ b/pkg/storage/installation_store_test.go @@ -165,7 +165,7 @@ func TestInstallationStorageProvider_Installations(t *testing.T) { defer cp.Close() t.Run("ListInstallations", func(t *testing.T) { - installations, err := cp.ListInstallations(context.Background(), "dev", "", nil) + installations, err := cp.ListInstallations(context.Background(), ListOptions{Namespace: "dev"}) require.NoError(t, err, "ListInstallations failed") require.Len(t, installations, 3, "Expected 3 installations") @@ -183,6 +183,36 @@ func TestInstallationStorageProvider_Installations(t *testing.T) { assert.Equal(t, cnab.StatusSucceeded, foo.Status.ResultStatus) }) + t.Run("ListInstallations with skip option", func(t *testing.T) { + installations, err := cp.ListInstallations(context.Background(), ListOptions{Namespace: "dev", Skip: 1}) + require.NoError(t, err, "ListInstallations failed") + + require.Len(t, installations, 2, "Expected 2 installations") + + bar := installations[0] + assert.Equal(t, "baz", bar.Name) + assert.Equal(t, cnab.StatusRunning, bar.Status.ResultStatus) + + baz := installations[1] + assert.Equal(t, "foo", baz.Name) + assert.Equal(t, cnab.StatusSucceeded, baz.Status.ResultStatus) + }) + + t.Run("ListInstallations with limit option", func(t *testing.T) { + installations, err := cp.ListInstallations(context.Background(), ListOptions{Namespace: "dev", Limit: 2}) + require.NoError(t, err, "ListInstallations failed") + + require.Len(t, installations, 2, "Expected 2 installations") + + bar := installations[0] + assert.Equal(t, "bar", bar.Name) + assert.Equal(t, cnab.StatusSucceeded, bar.Status.ResultStatus) + + baz := installations[1] + assert.Equal(t, "baz", baz.Name) + assert.Equal(t, cnab.StatusRunning, baz.Status.ResultStatus) + }) + t.Run("FindInstallations by label", func(t *testing.T) { opts := FindOptions{ Filter: map[string]interface{}{ @@ -236,14 +266,14 @@ func TestInstallationStorageProvider_DeleteInstallation(t *testing.T) { cp := generateInstallationData(t) defer cp.Close() - installations, err := cp.ListInstallations(context.Background(), "dev", "", nil) + installations, err := cp.ListInstallations(context.Background(), ListOptions{Namespace: "dev"}) 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) + installations, err = cp.ListInstallations(context.Background(), ListOptions{Namespace: "dev"}) require.NoError(t, err, "ListInstallations failed") assert.Len(t, installations, 2, "expected foo to be deleted") diff --git a/pkg/storage/migrations/manager_test.go b/pkg/storage/migrations/manager_test.go index 52830e1cd..e559ad27b 100644 --- a/pkg/storage/migrations/manager_test.go +++ b/pkg/storage/migrations/manager_test.go @@ -61,15 +61,15 @@ func TestManager_NoMigrationEmptyHome(t *testing.T) { defer mgr.Close() claimStore := storage.NewInstallationStore(mgr) - _, err := claimStore.ListInstallations(context.Background(), "", "", nil) + _, err := claimStore.ListInstallations(context.Background(), storage.ListOptions{}) require.NoError(t, err, "ListInstallations failed") credStore := storage.NewCredentialStore(mgr, nil) - _, err = credStore.ListCredentialSets(context.Background(), "", "", nil) + _, err = credStore.ListCredentialSets(context.Background(), storage.ListOptions{}) require.NoError(t, err, "List credentials failed") paramStore := storage.NewParameterStore(mgr, nil) - _, err = paramStore.ListParameterSets(context.Background(), "", "", nil) + _, err = paramStore.ListParameterSets(context.Background(), storage.ListOptions{}) require.NoError(t, err, "List credentials failed") } @@ -101,7 +101,7 @@ storage.Schema{ID:"schema", Installations:"needs-migration", Credentials:"1.0.1" } t.Run("list", func(t *testing.T) { - _, err = claimStore.ListInstallations(context.Background(), "", "", nil) + _, err = claimStore.ListInstallations(context.Background(), storage.ListOptions{}) checkMigrationError(t, err) }) @@ -124,7 +124,7 @@ func TestClaimStorage_NoMigrationRequiredForEmptyHome(t *testing.T) { defer mgr.Close() claimStore := storage.NewInstallationStore(mgr) - names, err := claimStore.ListInstallations(context.Background(), "", "", nil) + names, err := claimStore.ListInstallations(context.Background(), storage.ListOptions{}) require.NoError(t, err, "ListInstallations failed") assert.Empty(t, names, "Expected an empty list of installations since porter home is new") } @@ -156,7 +156,7 @@ storage.Schema{ID:"schema", Installations:"1.0.1", Credentials:"needs-migration" } t.Run("list", func(t *testing.T) { - _, err = credStore.ListCredentialSets(context.Background(), "", "", nil) + _, err = credStore.ListCredentialSets(context.Background(), storage.ListOptions{}) checkMigrationError(t, err) }) @@ -177,7 +177,13 @@ func TestCredentialStorage_NoMigrationRequiredForEmptyHome(t *testing.T) { testSecrets := secrets.NewTestSecretsProvider() credStore := storage.NewTestCredentialProviderFor(t, mgr, testSecrets) - names, err := credStore.ListCredentialSets(context.Background(), "", "", nil) + names, err := credStore.ListCredentialSets(context.Background(), storage.ListOptions{ + Namespace: "", + Name: "", + Labels: nil, + Skip: 0, + Limit: 0, + }) require.NoError(t, err, "List failed") assert.Empty(t, names, "Expected an empty list of credentials since porter home is new") } @@ -209,7 +215,7 @@ storage.Schema{ID:"schema", Installations:"1.0.1", Credentials:"1.0.1", Paramete } t.Run("list", func(t *testing.T) { - _, err = paramStore.ListParameterSets(context.Background(), "", "", nil) + _, err = paramStore.ListParameterSets(context.Background(), storage.ListOptions{}) checkMigrationError(t, err) }) @@ -230,7 +236,7 @@ func TestParameterStorage_NoMigrationRequiredForEmptyHome(t *testing.T) { testSecrets := secrets.NewTestSecretsProvider() paramStore := storage.NewTestParameterProviderFor(t, mgr, testSecrets) - names, err := paramStore.ListParameterSets(context.Background(), "", "", nil) + names, err := paramStore.ListParameterSets(context.Background(), storage.ListOptions{}) require.NoError(t, err, "List failed") assert.Empty(t, names, "Expected an empty list of parameters since porter home is new") } diff --git a/pkg/storage/parameter_store.go b/pkg/storage/parameter_store.go index e1c641a2f..658cf9e9d 100644 --- a/pkg/storage/parameter_store.go +++ b/pkg/storage/parameter_store.go @@ -100,12 +100,9 @@ func (s ParameterStore) InsertParameterSet(ctx context.Context, params Parameter return s.Documents.Insert(ctx, CollectionParameters, opts) } -func (s ParameterStore) ListParameterSets(ctx context.Context, namespace string, name string, labels map[string]string) ([]ParameterSet, error) { +func (s ParameterStore) ListParameterSets(ctx context.Context, listOptions ListOptions) ([]ParameterSet, error) { var out []ParameterSet - opts := FindOptions{ - Filter: CreateListFiler(namespace, name, labels), - } - err := s.Documents.Find(ctx, CollectionParameters, opts, &out) + err := s.Documents.Find(ctx, CollectionParameters, listOptions.ToFindOptions(), &out) return out, err } diff --git a/pkg/storage/parameter_store_test.go b/pkg/storage/parameter_store_test.go index 4f75eb53c..055390a64 100644 --- a/pkg/storage/parameter_store_test.go +++ b/pkg/storage/parameter_store_test.go @@ -14,7 +14,7 @@ func TestParameterStore_CRUD(t *testing.T) { defer paramStore.Close() ctx := context.Background() - params, err := paramStore.ListParameterSets(ctx, "dev", "", nil) + params, err := paramStore.ListParameterSets(ctx, ListOptions{Namespace: "dev"}) require.NoError(t, err) require.Empty(t, params, "Find should return no entries") @@ -32,16 +32,16 @@ func TestParameterStore_CRUD(t *testing.T) { err = paramStore.InsertParameterSet(ctx, myParamSet) require.NoError(t, err, "Insert should successfully save") - params, err = paramStore.ListParameterSets(ctx, "dev", "", nil) + params, err = paramStore.ListParameterSets(ctx, ListOptions{Namespace: "dev"}) require.NoError(t, err) require.Len(t, params, 1, "expected 1 parameter set") - require.Equal(t, myParamSet.Name, params[0].Name, "expected to retrieve myparams") + require.Equal(t, myParamSet.Name, params[0].Name, "expected to retrieve myparam") - params, err = paramStore.ListParameterSets(ctx, "", "", nil) + params, err = paramStore.ListParameterSets(ctx, ListOptions{}) require.NoError(t, err) require.Len(t, params, 0, "expected no global parameter sets") - params, err = paramStore.ListParameterSets(ctx, "*", "", nil) + params, err = paramStore.ListParameterSets(ctx, ListOptions{Namespace: "*"}) require.NoError(t, err) require.Len(t, params, 1, "expected 1 parameter set defined in all namespaces") @@ -49,16 +49,45 @@ func TestParameterStore_CRUD(t *testing.T) { require.NoError(t, err) require.Equal(t, myParamSet, pset, "Get should return the saved parameter set") + myParamSet2 := NewParameterSet("dev", "myparams2", + secrets.Strategy{ + Name: "myparam2", + Source: secrets.Source{ + Key: "value2", + Value: "myparamvalue2", + }, + }) + myParamSet2.Status.Created = time.Date(2020, 1, 1, 12, 0, 0, 0, time.UTC) + myParamSet2.Status.Modified = myParamSet2.Status.Created + + err = paramStore.InsertParameterSet(ctx, myParamSet2) + require.NoError(t, err, "Insert should successfully save") + + params, err = paramStore.ListParameterSets(ctx, ListOptions{Namespace: "dev", Skip: 1}) + require.NoError(t, err) + require.Len(t, params, 1, "expected 1 parameter set") + require.Equal(t, myParamSet2.Name, params[0].Name, "expected to retrieve myparam2") + + params, err = paramStore.ListParameterSets(ctx, ListOptions{Namespace: "dev", Limit: 1}) + require.NoError(t, err) + require.Len(t, params, 1, "expected 1 parameter set") + require.Equal(t, myParamSet.Name, params[0].Name, "expected to retrieve myparam") + err = paramStore.RemoveParameterSet(ctx, myParamSet.Namespace, myParamSet.Name) require.NoError(t, err, "Remove should successfully delete the parameter set") - params, err = paramStore.ListParameterSets(ctx, "dev", "", nil) + err = paramStore.RemoveParameterSet(ctx, myParamSet2.Namespace, myParamSet2.Name) + require.NoError(t, err, "Remove should successfully delete the parameter set") + + params, err = paramStore.ListParameterSets(ctx, ListOptions{Namespace: "dev"}) require.NoError(t, err) require.Empty(t, params, "List should return no entries") pset, err = paramStore.GetParameterSet(ctx, "", myParamSet.Name) require.ErrorIs(t, err, ErrNotFound{}) + pset, err = paramStore.GetParameterSet(ctx, "", myParamSet2.Name) + require.ErrorIs(t, err, ErrNotFound{}) } func TestParameterStorage_ResolveAll(t *testing.T) { diff --git a/pkg/storage/parameterset_provider.go b/pkg/storage/parameterset_provider.go index 86944f8ae..64b35c3d3 100644 --- a/pkg/storage/parameterset_provider.go +++ b/pkg/storage/parameterset_provider.go @@ -17,7 +17,7 @@ type ParameterSetProvider interface { Validate(ctx context.Context, params ParameterSet) error InsertParameterSet(ctx context.Context, params ParameterSet) error - ListParameterSets(ctx context.Context, namespace string, name string, labels map[string]string) ([]ParameterSet, error) + ListParameterSets(ctx context.Context, listOptions ListOptions) ([]ParameterSet, error) GetParameterSet(ctx context.Context, namespace string, name string) (ParameterSet, error) UpdateParameterSet(ctx context.Context, params ParameterSet) error UpsertParameterSet(ctx context.Context, params ParameterSet) error diff --git a/pkg/storage/query.go b/pkg/storage/query.go index 0e686d2ac..f19dab6cb 100644 --- a/pkg/storage/query.go +++ b/pkg/storage/query.go @@ -288,20 +288,45 @@ func convertToRawJsonDocument(in interface{}, raw interface{}) error { return json.Unmarshal(data, raw) } -// CreateListFiler builds a query for a list of documents by: -// * matching namespace -// * name contains substring -// * labels contains all matches -func CreateListFiler(namespace string, name string, labels map[string]string) map[string]interface{} { +// ListOptions is the set of options available to the list operation +// on any storage provider. +type ListOptions struct { + // Namespace in which the particular result list is defined. + Namespace string + + // Name specifies whether the result list name contain the specified substring. + Name string + + // Labels is used to filter result list based on a key-value pair. + Labels map[string]string + + // Skip is the number of results to skip past and exclude from the results. + Skip int64 + + // Limit is the number of results to return. + Limit int64 +} + +// ToFindOptions builds a query for a list of documents with these conditions: +// * sorted in ascending order by namespace first and then name +// * filtered by matching namespace, name contains substring, and labels contain all matches +// * skipped and limited to a certain number of result +func (o ListOptions) ToFindOptions() FindOptions { filter := make(map[string]interface{}, 3) - if namespace != "*" { - filter["namespace"] = namespace + if o.Namespace != "*" { + filter["namespace"] = o.Namespace } - if name != "" { - filter["name"] = map[string]interface{}{"$regex": name} + if o.Name != "" { + filter["name"] = map[string]interface{}{"$regex": o.Name} } - for k, v := range labels { + for k, v := range o.Labels { filter["labels."+k] = v } - return filter + + return FindOptions{ + Sort: []string{"namespace", "name"}, + Filter: filter, + Skip: o.Skip, + Limit: o.Limit, + } } diff --git a/pkg/storage/query_test.go b/pkg/storage/query_test.go index 8c93646bf..133b5e3f0 100644 --- a/pkg/storage/query_test.go +++ b/pkg/storage/query_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" ) var _ Document = TestDocument{} @@ -72,3 +73,27 @@ func TestFindOptions_ToPluginOptions(t *testing.T) { {"name", 1}} require.Equal(t, wantSortDoc, po.Sort) } + +func TestListOptions_ToFindOptions(t *testing.T) { + opts := ListOptions{ + Namespace: "dev", + Name: "name", + Labels: map[string]string{"key": "value"}, + Skip: 1, + Limit: 1, + } + + wantOpts := FindOptions{ + Sort: []string{"namespace", "name"}, + Skip: 1, + Limit: 1, + Filter: primitive.M{ + "labels.key": "value", + "name": map[string]interface{}{"$regex": "name"}, + "namespace": "dev", + }, + } + + gotOpts := opts.ToFindOptions() + require.Equal(t, wantOpts, gotOpts) +}