-
Notifications
You must be signed in to change notification settings - Fork 201
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 a porter explain command #646
Conversation
This PR adds a new `explain` command that prints usage information for a bundle Closes: getporter#635
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one real code change question; other comment just a thought/brainstorm. Otherwise, LGTM!
if err != nil { | ||
return err | ||
} | ||
err = p.ensureLocalBundleIsUpToDate(o.bundleFileOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking this call is not necessary, right? (Would re-build if bundle differed/out-of-date with local manifest.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, even though perhaps a main use case would be to explain a remote bundle, I can also see a use case for explaining a local bundle... in that scenario, we would want to ensure it'd be up-to-date. Therefore, seems we'd want to keep this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we for sure want to be able to use this on canonical JSON bundle files that are supper hard to read or a porter.yaml that people might have shared with others.
|
||
params := map[string]PrintableParameter{} | ||
for p, v := range bun.Parameters { | ||
def, ok := bun.Definitions[v.Definition] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea that this reminded me of whilst writin' similar code in Porter: We should add a helper func in cnab-go, GetDefinition(paramOrOutput)
or some such (may need a function for each type), that'd return a parameter's/output's definition or return the corresponding error (not found/empty/etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s a good idea. Do you want to file an issue for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yuuuup cnabio/cnab-go#134
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vdice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just some comments related to code consistency.
As a follow up, we should add this to the docs side bar
https://github.com/deislabs/porter/blob/7da5cffdf3581c27dfab03af88df2df005baa02d/docs/config.toml#L43-L47
@@ -0,0 +1,20 @@ | |||
Name: HELLO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there supposed to be an emoji here? I see a square :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this to use our pkg/printer, new table format is thus:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
@@ -221,7 +221,11 @@ defaultContentLanguage = "en" | |||
identifier = "wiring" | |||
weight = 303 | |||
parent = "authoring-bundles" | |||
|
|||
[[menu.main]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh my gosh! I hope you didn't think that I was trying to say that you should write net new docs! I mean that's great that you did, I was just saying that you should link to the autogenerated docs for porter explain
from the nav. Sorry about that if I was unclear. No worries, we can add the link in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i thought i added that one too :) sorrrry.
@@ -13,6 +13,11 @@ invocationImage: deislabs/porter-kubernetes:latest | |||
tag: deislabs/porter-kube-bundle:v0.1.0 | |||
|
|||
install: | |||
- exec: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to add this permanently to the example? Or was this accidentally included from our debugging session last night?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
embarassed emoji.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me go clean that up :)
Closes:
What does this change
This PR adds a new
explain
command that prints usage information for a bundle! Long gone are the days of trying to figure out how to use the bundle by running install until it works!This also provides support for
json
andyaml
output.An implementation note: I defined "Printable" types for this that only expose some of the fields of each type AND remove the omitempty on the json/yaml tags. I felt it was important to show values, even when they'd be omitted because they were "empty" (i.e. false...).
What issue does it fix
Closes #635
If there is not an existing issue, please make sure we have context on why this change is needed. See our Contributing Guide for examples of when an existing issue isn't necessary.
Notes for the reviewer
explain.go
file, because bundle is getting rather large and I think I'd eventually like to explode this command more and support like an "explain" command on parameters or outputs themselves, like print the JSON schema in a useful format so you could leverage that info when crafting parameters.Checklist