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

joshuabezaleel · 2022-06-06T08:18:15Z

What does this change

User that use the Porter CLI can use --skip and --limit flag for paginating their list output.

What issue does it fix

Closes #2051

Notes for the reviewer

Checklist

Did you write tests?
Did you write documentation?
Did you change porter.yaml or a storage document record? Update the corresponding schema file.
If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

Comment with /azp run test-porter-release if a magefile or build script was modified
Comment with /azp run porter-integration if it's a non-trivial PR

joshuabezaleel · 2022-06-06T08:19:04Z

@carolynvs Should we also make this --skip and --limit option available for porter credentials list and porter parameters list commands as well?

carolynvs

This looks great! I just have one suggestion that may help make the code easier to maintain over time.

carolynvs · 2022-06-08T12:59:03Z

pkg/storage/installation_store.go

@@ -63,14 +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) ([]Installation, error) {
+func (s InstallationStore) ListInstallations(ctx context.Context, namespace string, name string, labels map[string]string, skip int64, limit int64) ([]Installation, error) {


When the number of arguments for a function get long (and some are optional like skip/limit), I find it helpful to change the function so that it accepts an options struct. Then if we add more fields to the options struct in the future, we don't have to update every single place in the code where we call that function unless we need to set the new parameter value. This makes it easier to change the code over time.

What do you think about changing ListInstallations to this signature instead?

type ListInstallationOptions struct{ Namespace string Name string Labels map[string]string Skip int64 Limit int64 } func (s InstallationStore) ListInstallations(ctx context.Context, opts ListInstallationsOptions) ([]Installation, error) {

@carolynvs Actually been thinking about that as well! Will add it on the next commits.

Also, what do you think of making the --skip and --limit options available to porter credentials list and porter parameters list commands as well? If yes, I can also do the process of changing the function argument to option struct for those two as well.

If you want to do credentials/parameters either in this PR or a follow-up that would be great!

@carolynvs should be ready for another round of review! I made the option struct a general one (ListOptions) so that it can also be used for ListParameters and ListCredentialSets but looking forward to hearing your thoughts !

…mit flag Signed-off-by: joshuabezaleel <[email protected]>

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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>

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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>

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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>

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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>

Signed-off-by: Tanmay Chaudhry <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>

…n to credential and parameter list command as well Signed-off-by: joshuabezaleel <[email protected]>

carolynvs · 2022-06-10T18:04:40Z

pkg/storage/credential_store.go

 	var out []CredentialSet
 	opts := FindOptions{
-		Filter: CreateListFiler(namespace, name, labels),
+		Sort:   []string{"namespace", "name"},


Thanks for adding the sort!

carolynvs · 2022-06-10T18:06:43Z

pkg/storage/credential_store_test.go

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


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.

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!

pkg/storage/credential_store_test.go

carolynvs · 2022-06-10T18:07:21Z

pkg/storage/credential_store_test.go

 	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: "*",


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

carolynvs · 2022-06-10T18:10:51Z

pkg/storage/migrations/manager_test.go

@@ -177,7 +213,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{


In this case you can just pass an empty struct.

carolynvs · 2022-06-10T18:12:35Z

pkg/storage/parameter_store.go

 	var out []ParameterSet
 	opts := FindOptions{
-		Filter: CreateListFiler(namespace, name, labels),
+		Sort:   []string{"namespace", "name"},


suggestion: You don't have to do this but one way we can standardize all three of these list functions and make it easier to maintain going forward is to change CreateListFilter to a function like listOptions.ToFindOptions and it would handle setting the sort, filter, skip and limit.

That also crossed my mind since we write all of the same kind of input to the FindOptions to credential, parameter, and installation! Updated 🙂

carolynvs · 2022-06-10T18:13:10Z

pkg/storage/query.go

@@ -292,16 +292,36 @@ func convertToRawJsonDocument(in interface{}, raw interface{}) error {
 // * matching namespace
 // * name contains substring
 // * labels contains all matches
-func CreateListFiler(namespace string, name string, labels map[string]string) map[string]interface{} {
+// func CreateListFiler(namespace string, name string, labels map[string]string) map[string]interface{} {


Can you remove this commented out function signature?

Ah, sorry that this got left out. Thank you for pointing this out!

Signed-off-by: joshuabezaleel <[email protected]>

… receiver Signed-off-by: joshuabezaleel <[email protected]>

carolynvs

Oh this looks just great now! Thanks for the extra refactoring. 👍

…sult using skip and limit option (getporter#2137) * Add pagination option for installation list command using skip and limit flag Signed-off-by: joshuabezaleel <[email protected]> * 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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Add InstallationStore.FindInstallations (getporter#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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * 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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * 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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Add vet and lint targets to magefile Signed-off-by: Tanmay Chaudhry <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Add ListOption input parameter struct and enable skip and limit option to credential and parameter list command as well Signed-off-by: joshuabezaleel <[email protected]> * Leave out default value for ListOption's properties Signed-off-by: joshuabezaleel <[email protected]> * Remove commented function signature Signed-off-by: joshuabezaleel <[email protected]> * Convert CreateListFilter to ToFindOptions method for ListOptions type receiver Signed-off-by: joshuabezaleel <[email protected]> Co-authored-by: Carolyn Van Slyck <[email protected]> Co-authored-by: Tanmay Chaudhry <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>

* Use user specified directory for resolving file path (#2142) * use user specified build directory if provided for porter manifest Signed-off-by: Yingrong Zhao <[email protected]> * update tests Signed-off-by: Yingrong Zhao <[email protected]> * update doc and fix tests Signed-off-by: Yingrong Zhao <[email protected]> * address comment Signed-off-by: Yingrong Zhao <[email protected]> * explicitly set default value for o.Dir Signed-off-by: Yingrong Zhao <[email protected]> * clearer help text Signed-off-by: Yingrong Zhao <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Update to helm3 mixin v0.1.16 v0.1.16 includes fixes for using nonroot invocation images Signed-off-by: Carolyn Van Slyck <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Sanitize archive folder name (#2154) * fix archive folder creation Signed-off-by: Yingrong Zhao <[email protected]> * replace path separator instead Signed-off-by: Yingrong Zhao <[email protected]> * modify test Signed-off-by: Yingrong Zhao <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * 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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * 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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * 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 <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Add vet and lint targets to magefile Signed-off-by: Tanmay Chaudhry <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Add ListOption input parameter struct and enable skip and limit option to credential and parameter list command as well Signed-off-by: joshuabezaleel <[email protected]> * Leave out default value for ListOption's properties Signed-off-by: joshuabezaleel <[email protected]> * Remove commented function signature Signed-off-by: joshuabezaleel <[email protected]> * Convert CreateListFilter to ToFindOptions method for ListOptions type receiver Signed-off-by: joshuabezaleel <[email protected]> Co-authored-by: Carolyn Van Slyck <[email protected]> Co-authored-by: Tanmay Chaudhry <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Add state and status to list installation Signed-off-by: joshuabezaleel <[email protected]> * fix archive folder test Signed-off-by: Yingrong Zhao <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Fix Vet Errors (#2153) * Fix lint errors for unkeyed fields in composite literals Signed-off-by: Tanmay Chaudhry <[email protected]> * resolve lint errors on tags Signed-off-by: Tanmay Chaudhry <[email protected]> * Updated golden file to account for bad struct tag fix Signed-off-by: Tanmay Chaudhry <[email protected]> * Vet Fix: Rename example tests to use suffixes. Signed-off-by: Tanmay Chaudhry <[email protected]> * Replace ExtendedBundle{} initialization with a NewBundle constructor Signed-off-by: Tanmay Chaudhry <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Improve error message loading wrong schema (#2157) * Improve error message loading wrong schema Signed-off-by: Kevin Barbour <[email protected]> * Add myself to CONTRIBUTORS.MD Signed-off-by: Kevin Barbour <[email protected]> * Don't use errors pkg, fix assert in test Signed-off-by: Kevin Barbour <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Add prow github action This adds a prow github action that allows specified people (in the OWNERS file) to comment on a pull request with /lgtm to review the pull request, or /approve to merge the pull request. The github action handles executing the commands for you so that you don't need to have maintainer rights on the repository. Signed-off-by: Carolyn Van Slyck <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Switch prow to use pull_request instead of _target Signed-off-by: Carolyn Van Slyck <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Updated installation schema with correct dependency schema Signed-off-by: Steven Gettys <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * changed new manifest description for test Signed-off-by: Steven Gettys <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Update k8s and containerd dependencies * Update to cnab-go v0.23.4 * Update containerd to v1.6.6 * Updated k8s to v0.24.1. This does not update docker since buildkit uses a funny unreleased version of docker. We won't be able to update to a new version of Docker until there's a release that has the new feature that buildkit relies upon. See go.mod for a link to the troublesome package in question. Signed-off-by: Carolyn Van Slyck <[email protected]> Signed-off-by: joshuabezaleel <[email protected]> * Add comments Signed-off-by: joshuabezaleel <[email protected]> * StateDefined as default value Signed-off-by: joshuabezaleel <[email protected]> * Move displayinstallation's state and status to metadata Signed-off-by: joshuabezaleel <[email protected]> * Add golden file test for print installation Signed-off-by: joshuabezaleel <[email protected]> * Add unit test for displayInstallation's state and status Signed-off-by: joshuabezaleel <[email protected]> * Change function name from set to get Signed-off-by: joshuabezaleel <[email protected]> * Revert changes on test file Signed-off-by: joshuabezaleel <[email protected]> * add new line Signed-off-by: joshuabezaleel <[email protected]> * resolve conflict Signed-off-by: joshuabezaleel <[email protected]> * fix comment Signed-off-by: joshuabezaleel <[email protected]> Co-authored-by: Yingrong Zhao <[email protected]> Co-authored-by: Carolyn Van Slyck <[email protected]> Co-authored-by: Tanmay Chaudhry <[email protected]> Co-authored-by: Kevin Barbour <[email protected]> Co-authored-by: Steven Gettys <[email protected]>

joshuabezaleel requested review from carolynvs, jeremyrickard and vdice as code owners June 6, 2022 08:18

joshuabezaleel force-pushed the 2051-pagination-option-list branch from 0fc6fa1 to f0a7132 Compare June 6, 2022 08:21

joshuabezaleel mentioned this pull request Jun 7, 2022

Adding pagination for installation list result using skip and limit option #2136

Closed

4 tasks

carolynvs reviewed Jun 8, 2022

View reviewed changes

joshuabezaleel marked this pull request as draft June 10, 2022 11:02

joshuabezaleel and others added 7 commits June 10, 2022 23:32

Add pagination option for installation list command using skip and li…

ebac171

…mit flag Signed-off-by: joshuabezaleel <[email protected]>

Add vet and lint targets to magefile

ec77a4c

Signed-off-by: Tanmay Chaudhry <[email protected]> Signed-off-by: joshuabezaleel <[email protected]>

Add ListOption input parameter struct and enable skip and limit optio…

b1852a2

…n to credential and parameter list command as well Signed-off-by: joshuabezaleel <[email protected]>

joshuabezaleel force-pushed the 2051-pagination-option-list branch from fa607ea to b1852a2 Compare June 10, 2022 16:33

Merge branch 'release/v1' into 2051-pagination-option-list

7f0022e

joshuabezaleel changed the title ~~Adding pagination for installation list result using skip and limit option~~ Adding pagination for installation, parameter, and credential list result using skip and limit option Jun 10, 2022

joshuabezaleel marked this pull request as ready for review June 10, 2022 17:07

carolynvs reviewed Jun 10, 2022

View reviewed changes

joshuabezaleel added 3 commits June 11, 2022 13:29

Leave out default value for ListOption's properties

356e39f

Signed-off-by: joshuabezaleel <[email protected]>

Remove commented function signature

48b6b63

Signed-off-by: joshuabezaleel <[email protected]>

Convert CreateListFilter to ToFindOptions method for ListOptions type…

b3fbba7

… receiver Signed-off-by: joshuabezaleel <[email protected]>

joshuabezaleel force-pushed the 2051-pagination-option-list branch from e4ca9dd to b3fbba7 Compare June 11, 2022 06:29

carolynvs approved these changes Jun 15, 2022

View reviewed changes

carolynvs merged commit d7009a2 into getporter:release/v1 Jun 15, 2022

github-actions bot mentioned this pull request Jun 15, 2022

Add paging options to porter list and other commands that list installation data #2051

Closed

joshuabezaleel deleted the 2051-pagination-option-list branch June 19, 2022 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joshuabezaleel commented Jun 6, 2022

joshuabezaleel commented Jun 6, 2022

carolynvs left a comment

carolynvs Jun 8, 2022

joshuabezaleel Jun 9, 2022

carolynvs Jun 9, 2022

joshuabezaleel Jun 10, 2022 •

edited

Loading

carolynvs Jun 10, 2022

carolynvs Jun 10, 2022

joshuabezaleel Jun 11, 2022

carolynvs Jun 10, 2022

carolynvs Jun 10, 2022

carolynvs Jun 10, 2022

joshuabezaleel Jun 11, 2022

carolynvs Jun 10, 2022

joshuabezaleel Jun 11, 2022

carolynvs left a comment

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

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

Conversation

joshuabezaleel commented Jun 6, 2022

What does this change

What issue does it fix

Notes for the reviewer

Checklist

Reviewer Checklist

joshuabezaleel commented Jun 6, 2022

carolynvs 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

joshuabezaleel Jun 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

joshuabezaleel Jun 10, 2022 •

edited

Loading