Repository: getporter/porter
Pull request #2096 (2022-05-24 19:03:41)

Fix performance for porter list
# What does this change
Do not retrieve the full installation definition for each installation returned from list. We only need that information
for the show command. Right now porter is making multiple calls to both the storage and secrets plugin for each installation in the list results.

# What issue does it fix
Closes #2040

# Notes for the reviewer
Here is what the trace looks like with this fix (compared to the original trace which was huge)

![Screen Shot 2022-05-24 at 3 01 32 PM](https://user-images.githubusercontent.com/1368985/170122112-43edb282-cc4c-4c79-b9a4-4316ab0948cf.png)


# Checklist
- [x] 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][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

[contributors]: https://porter.sh/src/CONTRIBUTORS.md

carolynvs: @VinozzZ Okay I think I've fixed everything, please take another look!

Patch:
diff --git a/pkg/plugins/pluggable/loader.go b/pkg/plugins/pluggable/loader.go
index 53d3ad54..b89a240e 100644
--- a/pkg/plugins/pluggable/loader.go
+++ b/pkg/plugins/pluggable/loader.go
@@ -138,7 +138,7 @@ func (l *PluginLoader) readPluginConfig() (io.Reader, error) {
 
 	b, err := json.Marshal(l.selectedPluginConfig)
 	if err != nil {
-		return nil, errors.Wrapf(err, "could not marshal plugin config %#v", l.selectedPluginConfig)
+		return nil, errors.Wrapf(err, "could not marshal plugin config for %s", l.selectedPluginKey)
 	}
 
 	return bytes.NewBuffer(b), nil
diff --git a/pkg/porter/list.go b/pkg/porter/list.go
index 4aced87d..54a4bf75 100644
--- a/pkg/porter/list.go
+++ b/pkg/porter/list.go
@@ -214,31 +214,32 @@ func NewDisplayRun(run storage.Run) DisplayRun {
 }
 
 // ListInstallations lists installed bundles.
-func (p *Porter) ListInstallations(ctx context.Context, opts ListOptions) ([]storage.Installation, error) {
+func (p *Porter) ListInstallations(ctx context.Context, opts ListOptions) (DisplayInstallations, error) {
 	ctx, log := tracing.StartSpan(ctx)
 	defer log.EndSpan()
 
 	installations, err := p.Installations.ListInstallations(ctx, opts.GetNamespace(), opts.Name, opts.ParseLabels())
-	return installations, errors.Wrap(err, "could not list installations")
-}
-
-// PrintInstallations prints installed bundles.
-func (p *Porter) PrintInstallations(ctx context.Context, opts ListOptions) error {
-	installations, err := p.ListInstallations(ctx, opts)
 	if err != nil {
-		return err
+		return nil, log.Error(fmt.Errorf("could not list installations: %w", err))
 	}
 
 	var displayInstallations DisplayInstallations
 	for _, installation := range installations {
-		di, err := p.generateDisplayInstallation(ctx, installation)
-		if err != nil {
-			return err
-		}
+		di := NewDisplayInstallation(installation)
 		displayInstallations = append(displayInstallations, di)
 	}
 	sort.Sort(sort.Reverse(displayInstallations))
 
+	return displayInstallations, nil
+}
+
+// PrintInstallations prints installed bundles.
+func (p *Porter) PrintInstallations(ctx context.Context, opts ListOptions) error {
+	displayInstallations, err := p.ListInstallations(ctx, opts)
+	if err != nil {
+		return err
+	}
+
 	switch opts.Format {
 	case printer.FormatJson:
 		return printer.PrintJson(p.Out, displayInstallations)
diff --git a/pkg/porter/show.go b/pkg/porter/show.go
index 71b3548f..b529e460 100644
--- a/pkg/porter/show.go
+++ b/pkg/porter/show.go
@@ -41,30 +41,38 @@ func (so *ShowOptions) Validate(args []string, cxt *portercontext.Context) error
 }
 
 // GetInstallation retrieves information about an installation, including its most recent run.
-func (p *Porter) GetInstallation(ctx context.Context, opts ShowOptions) (storage.Installation, error) {
+func (p *Porter) GetInstallation(ctx context.Context, opts ShowOptions) (storage.Installation, *storage.Run, error) {
 	err := p.applyDefaultOptions(ctx, &opts.sharedOptions)
 	if err != nil {
-		return storage.Installation{}, err
+		return storage.Installation{}, nil, err
 	}
 
 	installation, err := p.Installations.GetInstallation(ctx, opts.Namespace, opts.Name)
 	if err != nil {
-		return storage.Installation{}, err
+		return storage.Installation{}, nil, err
 	}
 
-	return installation, nil
+	if installation.Status.RunID != "" {
+		run, err := p.Installations.GetRun(ctx, installation.Status.RunID)
+		if err != nil {
+			return storage.Installation{}, nil, err
+		}
+		return installation, &run, nil
+	}
+
+	return installation, nil, nil
 
 }
 
 // ShowInstallation shows a bundle installation, along with any
 // associated outputs
 func (p *Porter) ShowInstallation(ctx context.Context, opts ShowOptions) error {
-	installation, err := p.GetInstallation(ctx, opts)
+	installation, run, err := p.GetInstallation(ctx, opts)
 	if err != nil {
 		return err
 	}
 
-	displayInstallation, err := p.generateDisplayInstallation(ctx, installation)
+	displayInstallation, err := p.NewDisplayInstallationWithSecrets(ctx, installation, run)
 	if err != nil {
 		return err
 	}
@@ -163,27 +171,23 @@ func (p *Porter) ShowInstallation(ctx context.Context, opts ShowOptions) error {
 	}
 }
 
-func (p *Porter) generateDisplayInstallation(ctx context.Context, installation storage.Installation) (DisplayInstallation, error) {
-	run, err := p.Installations.GetRun(ctx, installation.Status.RunID)
-	if err != nil {
-		return DisplayInstallation{}, err
-	}
-
-	bun := cnab.ExtendedBundle{run.Bundle}
-	installParams, err := p.Sanitizer.RestoreParameterSet(ctx, installation.Parameters, bun)
-	if err != nil {
-		return DisplayInstallation{}, err
-	}
-	runParams, err := p.Sanitizer.RestoreParameterSet(ctx, run.Parameters, bun)
-	if err != nil {
-		return DisplayInstallation{}, err
-	}
-
+func (p *Porter) NewDisplayInstallationWithSecrets(ctx context.Context, installation storage.Installation, run *storage.Run) (DisplayInstallation, error) {
 	displayInstallation := NewDisplayInstallation(installation)
-	displayInstallation.Parameters = installParams
 
-	displayInstallation.ResolvedParameters = NewDisplayValuesFromParameters(bun, runParams)
+	if run != nil {
+		bun := cnab.ExtendedBundle{run.Bundle}
+		installParams, err := p.Sanitizer.RestoreParameterSet(ctx, installation.Parameters, bun)
+		if err != nil {
+			return DisplayInstallation{}, err
+		}
+		displayInstallation.Parameters = installParams
+
+		runParams, err := p.Sanitizer.RestoreParameterSet(ctx, run.Parameters, bun)
+		if err != nil {
+			return DisplayInstallation{}, err
+		}
+		displayInstallation.ResolvedParameters = NewDisplayValuesFromParameters(bun, runParams)
+	}
 
 	return displayInstallation, nil
-
 }


Test patch:
diff --git a/pkg/porter/examples/install_example_test.go b/pkg/porter/examples/install_example_test.go
index 79896d3c..bf33fb2c 100644
--- a/pkg/porter/examples/install_example_test.go
+++ b/pkg/porter/examples/install_example_test.go
@@ -37,7 +37,7 @@ func ExampleInstall() {
 		log.Fatal(err)
 	}
 
-	installation, err := p.GetInstallation(context.Background(), showOpts)
+	installation, _, err := p.GetInstallation(context.Background(), showOpts)
 	if err != nil {
 		log.Fatal(err)
 	}
diff --git a/pkg/porter/list_test.go b/pkg/porter/list_test.go
index 815ed8bf..63403d18 100644
--- a/pkg/porter/list_test.go
+++ b/pkg/porter/list_test.go
@@ -53,10 +53,17 @@ func TestNewDisplayInstallation(t *testing.T) {
 }
 
 func TestPorter_ListInstallations(t *testing.T) {
+	ctx := context.Background()
 	p := NewTestPorter(t)
 	defer p.Close()
 
-	p.TestInstallations.CreateInstallation(storage.NewInstallation("", "shared-mysql"))
+	i1 := storage.NewInstallation("", "shared-mysql")
+	i1.Parameters.Parameters = []secrets.Strategy{ // Define a parameter that is stored in a secret, list should not retrieve it
+		{Name: "password", Source: secrets.Source{Key: "secret", Value: "mypassword"}},
+	}
+	i1.Status.RunID = "10" // Add a run but don't populate the data for it, list should not retrieve it
+
+	p.TestInstallations.CreateInstallation(i1)
 	p.TestInstallations.CreateInstallation(storage.NewInstallation("dev", "carolyn-wordpress"))
 	p.TestInstallations.CreateInstallation(storage.NewInstallation("dev", "vaughn-wordpress"))
 	p.TestInstallations.CreateInstallation(storage.NewInstallation("test", "staging-wordpress"))
@@ -65,26 +72,31 @@ func TestPorter_ListInstallations(t *testing.T) {
 
 	t.Run("all-namespaces", func(t *testing.T) {
 		opts := ListOptions{AllNamespaces: true}
-		results, err := p.ListInstallations(context.Background(), opts)
+		results, err := p.ListInstallations(ctx, opts)
 		require.NoError(t, err)
 		assert.Len(t, results, 6)
+
+		// Check that porter didn't go off and retrieve extra data for each installation
+		for _, r := range results {
+			assert.Empty(t, r.ResolvedParameters, "ListInstallations should not resolve secrets used by the installations")
+		}
 	})
 
 	t.Run("local namespace", func(t *testing.T) {
 		opts := ListOptions{Namespace: "dev"}
-		results, err := p.ListInstallations(context.Background(), opts)
+		results, err := p.ListInstallations(ctx, opts)
 		require.NoError(t, err)
 		assert.Len(t, results, 2)
 
 		opts = ListOptions{Namespace: "test"}
-		results, err = p.ListInstallations(context.Background(), opts)
+		results, err = p.ListInstallations(ctx, opts)
 		require.NoError(t, err)
 		assert.Len(t, results, 3)
 	})
 
 	t.Run("global namespace", func(t *testing.T) {
 		opts := ListOptions{Namespace: ""}
-		results, err := p.ListInstallations(context.Background(), opts)
+		results, err := p.ListInstallations(ctx, opts)
 		require.NoError(t, err)
 		assert.Len(t, results, 1)
 	})
diff --git a/pkg/porter/show_test.go b/pkg/porter/show_test.go
index be0471ae..1a94d40a 100644
--- a/pkg/porter/show_test.go
+++ b/pkg/porter/show_test.go
@@ -13,7 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestPorter_ShowBundle(t *testing.T) {
+func TestPorter_ShowInstallationWithBundle(t *testing.T) {
 	t.Parallel()
 
 	ref := "getporter/wordpress:v0.1.0"
@@ -141,3 +141,85 @@ func TestPorter_ShowBundle(t *testing.T) {
 		})
 	}
 }
+
+func TestPorter_ShowInstallationWithoutRecordedRun(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	p := NewTestPorter(t)
+	defer p.Close()
+
+	opts := ShowOptions{
+		sharedOptions: sharedOptions{
+			Namespace: "dev",
+			Name:      "mywordpress",
+		},
+		PrintOptions: printer.PrintOptions{
+			Format: printer.FormatPlaintext,
+		},
+	}
+
+	// Create test runs
+	writeOnly := true
+	b := bundle.Bundle{
+		Name:    "wordpress",
+		Version: "0.1.0",
+		Definitions: definition.Definitions{
+			"secretString": &definition.Schema{
+				Type:      "string",
+				WriteOnly: &writeOnly,
+			},
+			"bar": &definition.Schema{
+				Type: "string",
+			},
+			"logLevel": &definition.Schema{
+				Type: "integer",
+			},
+		},
+		Parameters: map[string]bundle.Parameter{
+			"logLevel": {
+				Definition: "logLevel",
+			},
+			"token": {
+				Definition: "foo",
+			},
+			"secretString": {
+				Definition: "secretString",
+			},
+		},
+		Outputs: map[string]bundle.Output{
+			"foo": {
+				Definition: "secretString",
+				Path:       "/path/to/foo",
+			},
+			"bar": {
+				Definition: "bar",
+			},
+		},
+	}
+
+	bun := cnab.ExtendedBundle{b}
+	p.TestInstallations.CreateInstallation(storage.NewInstallation("dev", "mywordpress"), p.TestInstallations.SetMutableInstallationValues, func(i *storage.Installation) {
+		i.TrackBundle(cnab.MustParseOCIReference("getporter/wordpress:v0.1.0"))
+		i.Labels = map[string]string{
+			"io.cnab/app":        "wordpress",
+			"io.cnab/appVersion": "v1.2.3",
+		}
+		params := []secrets.Strategy{
+			{Name: "logLevel", Source: secrets.Source{Value: "3"}, Value: "3"},
+			secrets.Strategy{Name: "secretString", Source: secrets.Source{Key: "secretString", Value: "foo"}, Value: "foo"},
+		}
+		i.Parameters = i.NewInternalParameterSet(params...)
+
+		i.ParameterSets = []string{"dev-env"}
+
+		i.Parameters.Parameters = p.SanitizeParameters(i.Parameters.Parameters, i.ID, bun)
+	})
+
+	// do not create a run, simulate that the installation failed before the bundle could execute (or show was called before the bundle was run)
+
+	err := p.ShowInstallation(ctx, opts)
+	require.NoError(t, err, "ShowInstallation failed")
+	p.CompareGoldenFile("testdata/show/bundle-never-run.txt", p.TestConfig.TestContext.GetOutput())
+
+}
diff --git a/pkg/porter/testdata/show/bundle-never-run.txt b/pkg/porter/testdata/show/bundle-never-run.txt
new file mode 100644
index 00000000..430631c8
--- /dev/null
+++ b/pkg/porter/testdata/show/bundle-never-run.txt
@@ -0,0 +1,22 @@
+Name: mywordpress
+Namespace: dev
+Created: 2020-04-18
+Modified: 2020-04-18
+
+Bundle:
+  Repository: getporter/wordpress
+  Version: 0.1.0
+
+Labels:
+  io.cnab/app: wordpress
+  io.cnab/appVersion: v1.2.3
+
+Parameter Sets:
+  - dev-env
+
+Status:
+  Reference: 
+  Version: 
+  Last Action: 
+  Status: 
+  Digest: