-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Help guides link #2857
Help guides link #2857
Conversation
Will display when user types `docker help` or `docker --help`, but not for `docker run --help`. Signed-off-by: Guillaume Tardif <guillaume.tardif@gmail.com>
Signed-off-by: Guillaume Tardif <guillaume.tardif@gmail.com>
Signed-off-by: Guillaume Tardif <guillaume.tardif@gmail.com>
cli/cobra.go
Outdated
@@ -360,6 +376,9 @@ Invalid Plugins: | |||
|
|||
Run '{{.CommandPath}} COMMAND --help' for more information on a command. | |||
{{- end}} | |||
{{- if displayHelpLink .}} | |||
{{ cyan "To get more help with docker, check out guides at https://docs.docker.com/go/guides/" }} |
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.
Wondering (discussed that option briefly with @chris-crone) if instead of hard-coding, we should use annotations similar to; https://github.com/docker/cli/blob/v20.10.0-rc1/cli/cobra.go#L195-L198
That way we could just print the information if its set on a specific (or "any") command
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.
+1 to this but we can also do it as a follow up if we add links for specific helps
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.
Right, added the additional help message in root annotation, so we can add specific messages to any command as we like in the future. (also renamed methods for something more homogeneous)
Codecov Report
@@ Coverage Diff @@
## master #2857 +/- ##
==========================================
- Coverage 57.10% 57.07% -0.04%
==========================================
Files 297 297
Lines 18635 18646 +11
==========================================
Hits 10642 10642
- Misses 7134 7144 +10
- Partials 859 860 +1 |
cli/cobra.go
Outdated
@@ -47,6 +49,8 @@ func setupCommonRootCommand(rootCmd *cobra.Command) (*cliflags.ClientOptions, *p | |||
rootCmd.PersistentFlags().MarkShorthandDeprecated("help", "please use --help") | |||
rootCmd.PersistentFlags().Lookup("help").Hidden = true | |||
|
|||
rootCmd.Annotations = map[string]string{"additionalHelp": cyan("To get more help with docker, check out guides at https://docs.docker.com/go/guides/")} |
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.
super nit: @thaJeztah do we need the trailing slash
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.
Two questions but LGTM
Signed-off-by: Guillaume Tardif <guillaume.tardif@gmail.com>
d13c1f7
to
d7697f9
Compare
@thaJeztah should be OK now I guess ; still dependent on merging the redirect in docs.docker.com, though (and @chris-crone comment on the trailing slash). |
@thaJeztah Got the OK from ben on this. Can we remove the trailing slash? |
LGTM ship 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.
LGTM
- What I did
link actually depends on docker/docs#11812
- How I did it
Added section at the bottom of go template, and a function to check if it should display or not
- How to verify it
docker help
will display it,docker --help
also, but notdocker run --help
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)