Only include required install params in explain command #1875
Conversation
|
/azp run porter-integration |
|
Azure Pipelines successfully started running 1 pipeline(s). |
When porter explain prints a suggested installation command, it should only include parameters that are required _and_ apply to the install command. Parameters can be limited to only apply to a particular action and if it doesn't apply to install, we don't want to tell people to specify in for porter install. Signed-off-by: Carolyn Van Slyck <[email protected]>
carolynvs
force-pushed
the
fix-explain-install-cmd
branch
from
0e5a31d to
65fa0e6
Compare
| @@ -230,6 +231,7 @@ func generatePrintable(bun cnab.ExtendedBundle, action string) (*PrintableBundle | |||
| sort.Sort(SortPrintableCredential(pb.Credentials)) | |||
|
|
|||
| for p, v := range bun.Parameters { | |||
| v := v // Go closures are funny like that | |||
There was a problem hiding this comment.
@carolynvs May I know the reason behind this one? Might be something that I haven't known yet about Go 😕
There was a problem hiding this comment.
Yeah it's super obscure and a really annoying part of how Go handles "closures". If you search for "golang for loop closure" you'll find a lot of articles on it.
Without this hack, when I save off a reference to v on the display parameter, it ends up with all parameters having a reference to the same address instead of each display parameter getting a reference to its proper parameter.
There was a problem hiding this comment.
Ah, I just realized they mentioned it on the official repo. It's this one, right? Thank you for the nugget of knowledge, Carolyn! Appreciate it 😄
There was a problem hiding this comment.
Yup! That is the problem I was working around. 👍
What does this change
When porter explain prints a suggested installation command, it should
only include parameters that are required and apply to the install
command. Parameters can be limited to only apply to a particular action
and if it doesn't apply to install, we don't want to tell people to
specify in for porter install.
What issue does it fix
I found this when running porter explain on the operator bundle, which has a parameter that only applies to the configureNamespace action.
Notes for the reviewer
N/A
Checklist
Reviewer Checklist