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.

Sign up for GitHub

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

Jump to bottom

Add state and status to list installation #2171

Merged
merged 24 commits into from
Jun 23, 2022
Merged

Add state and status to list installation #2171

merged 24 commits into from
Jun 23, 2022

Conversation

joshuabezaleel
Copy link
Contributor

What does this change

Add state and status to porter installation list command and tweak the columns.

$ porter list
NAMESPACE   NAME             VERSION    STATE           STATUS      MODIFIED
dev         something        1.2.3      installed       failed      7 seconds ago
dev         wordpress        1.2.0      defined         installing  4 minutes ago
dev         porter-hello     0.1.1      uninstalled     success     3 hours ago

What issue does it fix

Closes #1853

Notes for the reviewer

Put any questions or notes for the reviewer here.

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 joshuabezaleel force-pushed the 1853-installation-improvement branch from c7658c8 to 47eb8e4 Compare June 18, 2022 07:54
@joshuabezaleel joshuabezaleel marked this pull request as draft June 18, 2022 07:54
@joshuabezaleel joshuabezaleel force-pushed the 1853-installation-improvement branch from 183eaee to f29d991 Compare June 18, 2022 14:45
@joshuabezaleel
Copy link
Contributor Author

@carolynvs There are 2 things that I am still not sure of:

  1. On is it okay that I added 2 properties to the DisplayInstallation since it still needs to use method and properties from the storage.Installation and
  2. Whether we need and are able to add test for comparing golden file test for the new display of installation list.

Looking forward to the review! 😄

@joshuabezaleel
Copy link
Contributor Author

image

@joshuabezaleel joshuabezaleel marked this pull request as ready for review June 18, 2022 14:49
@carolynvs
Copy link
Member

is it okay that I added 2 properties to the DisplayInstallation since it still needs to use method and properties from the storage.Installation

Absolutely, I didn't expect that we would have all the necessary information already in the display data structures.

Whether we need and are able to add test for comparing golden file test for the new display of installation list.

You're right we are missing a test case for the human readable output of the porter list command! Can you add that and a golden file for its output?

carolynvs
carolynvs requested changes Jun 21, 2022
View reviewed changes
pkg/porter/list.go Outdated Show resolved Hide resolved
pkg/porter/list.go Show resolved Hide resolved
@carolynvs carolynvs self-assigned this Jun 21, 2022
@joshuabezaleel joshuabezaleel force-pushed the 1853-installation-improvement branch from 9bdae33 to c2a35aa Compare June 23, 2022 08:15
VinozzZ and others added 16 commits June 23, 2022 15:36
@VinozzZ @joshuabezaleel
Use user specified directory for resolving file path (getporter#2142)
e9e87b0 
* 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]>
@carolynvs @joshuabezaleel
Update to helm3 mixin v0.1.16
7faa23f 
v0.1.16 includes fixes for using nonroot invocation images

Signed-off-by: Carolyn Van Slyck <[email protected]>
Signed-off-by: joshuabezaleel <[email protected]>
@VinozzZ @joshuabezaleel
Sanitize archive folder name (getporter#2154)
4b40a1d 
* 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]>
@joshuabezaleel @carolynvs @tchaudhry91
Adding pagination for installation, parameter, and credential list re…
c8f3279 
…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]>
@joshuabezaleel
Add state and status to list installation
0443a86 
Signed-off-by: joshuabezaleel <[email protected]>
@VinozzZ @joshuabezaleel
fix archive folder test
a9d285d 
Signed-off-by: Yingrong Zhao <[email protected]>
Signed-off-by: joshuabezaleel <[email protected]>
@tchaudhry91 @joshuabezaleel
Fix Vet Errors (getporter#2153)
6015462 
* 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]>
@kevinbarbour @joshuabezaleel
Improve error message loading wrong schema (getporter#2157)
22917e6 
* 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]>
@carolynvs @joshuabezaleel
Add prow github action
3e2700d 
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]>
@carolynvs @joshuabezaleel
Switch prow to use pull_request instead of _target
94a67e3 
Signed-off-by: Carolyn Van Slyck <[email protected]>
Signed-off-by: joshuabezaleel <[email protected]>
@joshuabezaleel
Updated installation schema with correct dependency schema
ca4fc28 
Signed-off-by: Steven Gettys <[email protected]>
Signed-off-by: joshuabezaleel <[email protected]>
@joshuabezaleel
changed new manifest description for test
6338fc7 
Signed-off-by: Steven Gettys <[email protected]>
Signed-off-by: joshuabezaleel <[email protected]>
@carolynvs @joshuabezaleel
Update k8s and containerd dependencies
52bf273 
* 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]>
@joshuabezaleel
Add comments
e96235a 
Signed-off-by: joshuabezaleel <[email protected]>
@joshuabezaleel
StateDefined as default value
4062b1a 
Signed-off-by: joshuabezaleel <[email protected]>
@joshuabezaleel
Move displayinstallation's state and status to metadata
ba643f3 
Signed-off-by: joshuabezaleel <[email protected]>
@joshuabezaleel joshuabezaleel force-pushed the 1853-installation-improvement branch from ef61894 to 9ede1cd Compare June 23, 2022 08:36
@joshuabezaleel joshuabezaleel force-pushed the 1853-installation-improvement branch from 77ed640 to 2c92630 Compare June 23, 2022 08:52
@joshuabezaleel
resolve conflict
b60a7a7 
Signed-off-by: joshuabezaleel <[email protected]>
@joshuabezaleel joshuabezaleel force-pushed the 1853-installation-improvement branch from 41525f0 to b60a7a7 Compare June 23, 2022 08:55
@joshuabezaleel
fix comment
8ea1073 
Signed-off-by: joshuabezaleel <[email protected]>
@joshuabezaleel joshuabezaleel force-pushed the 1853-installation-improvement branch from 3374c53 to 8ea1073 Compare June 23, 2022 09:10
@joshuabezaleel
Copy link
Contributor Author

@carolynvs I hope this should be ready for another round of review 😁

carolynvs
carolynvs approved these changes Jun 23, 2022
View reviewed changes
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

This looks great, thank you! 🎉

@carolynvs carolynvs merged commit 5d111f1 into getporter:release/v1 Jun 23, 2022
@github-actions github-actions bot mentioned this pull request Jun 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carolynvs carolynvs carolynvs approved these changes

@jeremyrickard jeremyrickard Awaiting requested review from jeremyrickard

@vdice vdice Awaiting requested review from vdice

Assignees

@carolynvs carolynvs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joshuabezaleel @carolynvs @VinozzZ @tchaudhry91 @kevinbarbour