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.

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

Add installation instruction to porter explain command #1773

Conversation

joshuabezaleel
Copy link
Contributor

What does this change

Add installation instructions for porter explain command which includes generating credentials first and then including required parameters to the installation instruction of a particular bundle.

image

What issue does it fix

Closes #1511

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

  • Unit Tests
  • Documentation
  • Schema (porter.yaml)

@joshuabezaleel
Copy link
Contributor Author

Hi @carolynvs I've added a draft PR for this particular issue and a screenshot showing the result of running the command.
Should I also add a new unit test using a new bundle json test file for this specific case?

Thank you so much in advance! Looking forward to the review 🙂

@carolynvs
Copy link
Member

carolynvs commented Sep 22, 2021

Since you already updated the existing unit test's output, there's no need for a new unit test. 👍

@joshuabezaleel joshuabezaleel force-pushed the explain-command-installation-instruction branch from cfd0294 to 2cda5c3 Compare September 23, 2021 03:32
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.

Thanks, this is exactly what I was looking for. I have just some small revisions but it's looking good!

carolynvs and others added 11 commits October 15, 2021 16:46
Installation.Parameters should reflect parameter values that the user
selected, similar to --param on the command line. It shouldn't contain
any resolved values, such as from parameter sets as we already track
those on the Installation.ParameterSets field.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
When we output an installation with porter installation show, the data
available in the plaintext version should be comparable to what's in the
json/yaml output. Now that we are displaying all resolved parameters,
but only storing the user-provided parameters on the installation
record, we need to output more than just the installation record for
those output formats.

I have changed DisplayInstallation to better wrap an Installation
record, and then put all calculated fields into _calculated so that
people can easily tell which fields are associated with the record (and
can be changed when imported) and which ones are informational only.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
When I refactored, it causes a bunch of errors when running a bundle
with dependencies because I wasn't creating an explicit installation
record for the dependency.

I have updated the code so that an installation is created and labled
with

sh.porter.parentInstallation = installation namespace/name

so that later we can filter them out if we want to.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Print the params and creds passed to a bundle, including their values.
Make sure that sensitive values are censored.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
I had commented these out while refactoring, and forgot to add them back
when I was done.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
The porter state file is not always present for
bundles that do not use state. So skip over it when
it's not found as a parameter.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Do not include internal outputs, such as porter-state.tgz when listing
outputs.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
…command

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
@joshuabezaleel joshuabezaleel force-pushed the explain-command-installation-instruction branch from 613ae08 to 8547ecc Compare October 15, 2021 09:47
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
@joshuabezaleel joshuabezaleel force-pushed the explain-command-installation-instruction branch from 1c6c27c to f54812e Compare October 15, 2021 09:54
@carolynvs
Copy link
Member

I just checked out the latest code and tried it out:

$ porter explain -r getporter/wordpress:v0.1.4
Name: wordpress
Description:
Version: 0.1.4
Porter Version: v1.0.0-alpha.3

Credentials:
Name         Description   Required   Applies To
kubeconfig                 true       All Actions

Parameters:
Name                 Description   Type     Default               Required   Applies To
namespace                          string   wordpress             false      All Actions
wordpress-name                     string   porter-ci-wordpress   false      All Actions
wordpress-password                 string   <nil>                 true       install,upgrade

Outputs:
Name                 Description                           Type     Applies To
wordpress-password   The Wordpress installation password   string   install,upgrade

Actions:
Name   Description   Modifies Installation   Stateless
ping   ping          true                    false

Dependencies:
Alias   Reference
mysql   localhost:5000/mysql:v0.1.4

This bundle uses the following tools: exec, helm3.
To install this bundle run the following command, passing --param KEY=VALUE for any parameters you want to customize:
porter credentials generate kubeconfig --reference getporter/wordpress:v0.1.4
porter install --reference getporter/wordpress:v0.1.4wordpress-password=TODO  --param  --cred kubeconfig

It's super close, there are just two things that need fixing:

  • Let's start the instructions on it's own line, so that it's in a separate paragraph. Right now it kind of blends in with "This bundle uses the following tools"
  • The parameters are printing out incorrectly. First because there isn't a space between the bundle reference and the parameter, but also because it should be --param wordpress-password=TODO, right now the flag value is printed before the flag.

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
@joshuabezaleel joshuabezaleel force-pushed the explain-command-installation-instruction branch from 943df85 to 673d566 Compare October 15, 2021 21:19
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.

Looks great, thanks for sticking with it. This is going to make it a lot easier for people new to Porter. 💯

$ porter explain -r getporter/porter-hello:v0.1.1
Name: porter-hello
Description: An example Porter configuration
Version: 0.1.1
Porter Version: v1.0.0-alpha.3

This bundle uses the following tools: exec.

To install this bundle run the following command, passing --param KEY=VALUE for any parameters you want to customize:
porter install --reference getporter/porter-hello:v0.1.1

@carolynvs carolynvs merged commit e18feae into getporter:release/v1 Oct 15, 2021
@carolynvs
Copy link
Member

I'm working on a new alpha release, so this will be included when it goes out next week!

@joshuabezaleel
Copy link
Contributor Author

Hey @carolynvs , sorry that I was too tired yesterday because I just got back from a trip so I left the PR kind of hanging uncomfortably in the middle of doing it, and I am also not sure why 1 one of my commits got deleted while resolving the conflicts, thus the failing tests 🤔 Thank you for the on-point and detailed review!

Here is the screenshot of the explain command using the getporter/wordpress reference that you've used as the example earlier, also fixed the order of the --param flag.

Thank you so much for responding and merging the PR this fast! 😄

image

@joshuabezaleel joshuabezaleel deleted the explain-command-installation-instruction branch October 15, 2021 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants