Fix performance for porter list #2096
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Do not retrieve the full installatin definition for each installation returned from list. We only need that information for the show command. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
carolynvs
commented
May 24, 2022
| if err != nil { | ||
| return err | ||
| } | ||
| di := NewDisplayInstallation(installation) |
There was a problem hiding this comment.
I moved the conversion of installations to display installations so that we could test these functions. In other parts of Porter we do the same thing but I guess this function is old enough it was doing the conversion in the print function still.
VinozzZ
reviewed
May 25, 2022
pkg/porter/list.go
Outdated
| if err != nil { | ||
| return err | ||
| return nil, errors.Wrap(err, "could not list installations") |
There was a problem hiding this comment.
Do we want to trace the error here?
pkg/porter/show.go
Outdated
| return DisplayInstallation{}, err | ||
| } | ||
|
|
||
| func (p *Porter) NewDisplayInstallationWithSecrets(ctx context.Context, installation storage.Installation, run *storage.Run) (DisplayInstallation, error) { | ||
| bun := cnab.ExtendedBundle{run.Bundle} |
There was a problem hiding this comment.
What if the run is nil here?
There was a problem hiding this comment.
Haha you caught that at the same time I did, I was fixing that right before our meeting this morning. I just pushed it up now.
It's possible for an installation record to be created but the bundle isn't run before porter show is called. I've fixed how we put together a DisplayInstallation to account for that. Otherwise that code is returning a nil pointer exception. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
|
@VinozzZ Okay I think I've fixed everything, please take another look! |
VinozzZ
approved these changes
May 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)
Checklist
Reviewer Checklist