Only include required install params in explain command#1875
Only include required install params in explain command#1875carolynvs merged 1 commit intogetporter:release/v1from
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 <me@carolynvanslyck.com>
carolynvs
force-pushed
the
fix-explain-install-cmd
branch
from
0e5a31d to
65fa0e6
Compare
| 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. 👍
3 participants
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