-
Notifications
You must be signed in to change notification settings - Fork 2k
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
cmd: set double quotes as code delimiter #3924
Conversation
5982a37
to
faf3ce3
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3924 +/- ##
==========================================
- Coverage 59.20% 59.20% -0.01%
==========================================
Files 287 287
Lines 24687 24690 +3
==========================================
Hits 14617 14617
- Misses 9186 9189 +3
Partials 884 884 |
faf3ce3
to
5d6abd3
Compare
@thaJeztah I wonder if https://github.com/docker/cli/blob/master/docs/reference/commandline/dockerd.md should not be in moby repo instead? |
Yeah, there was a bit of a debate at the time about that. I suggested it should be in the moby repo, but ISTR there were some reasons at the time to move it as well (but it may have been "it's easier to move all files"). We should look at moving it back, but ideally preserving history (might be fun as it will contain history from "before" it was moved and "after", and possibly because of the move; same history twice 😅 😓). |
5d6abd3
to
7146630
Compare
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! Overall looking great! Left two comments (and opened a PR 😄 )
7146630
to
7bd4eb2
Compare
cli/command/node/update.go
Outdated
flags.StringVar(&options.availability, flagAvailability, "", `Availability of the node ("active"|"pause"|"drain")`) | ||
flags.Var(&options.annotations.labels, flagLabelAdd, "Add or update a node label (key=value)") | ||
flags.StringVar(&options.role, flagRole, "", `Role of the node ("worker|manager")`) | ||
flags.StringVar(&options.availability, flagAvailability, "", `Availability of the node ("active|pause|drain")`) |
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 slightly on the fence on these (but admitted: we weren't very consistent); slightly wondering what's clearer on intent;
"foo"|"bar"
either"foo"
(string) or"bar"
(string)"foo|bar"
a string of either foo or bar- some other notation? (
"foo", "bar", or "baz"
) ? - extended notation for all of these (multiple lines; short explanation for each option)
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.
in the build docs we've used comma-separated notation, and the options wrapped in code spans. My subjective opinion is that comma is more readable than pipes, but this is probably debatable.
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.
IMO single quotes containing all options are confusing ("foo|bar"
).
Personally I find commas clearer than pipes when only one value can be given at once, so 3rd option looks good for me:
flags.StringVar(&options.role, flagRole, "", `Role of the node ("worker" or "manager")`)
flags.StringVar(&options.availability, flagAvailability, "", `Availability of the node ("active", "pause" or "drain")`)
7bd4eb2
to
cf59011
Compare
cf59011
to
35af679
Compare
Looks like an e2e test needs updating;
|
Keep frontmatter for docker, dockerd and index markdown files. Also needs to copy docker.md > cli.md otherwise yaml generation on docs website doesn't work: docker#3924 (comment) Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Keep frontmatter for docker, dockerd and index markdown files. Also needs to copy docker.md > cli.md otherwise yaml generation on docs website doesn't work: docker#3924 (comment) Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
a17bc0f
to
b7d9555
Compare
Keep frontmatter for docker, dockerd and index markdown files. Also needs to copy docker.md > cli.md otherwise yaml generation on docs website doesn't work: docker#3924 (comment) Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Keep frontmatter for docker, dockerd and index markdown files. Also needs to copy docker.md > cli.md otherwise yaml generation on docs website doesn't work: docker#3924 (comment) Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Keep frontmatter for docker, dockerd and index markdown files. Also needs to copy docker.md > cli.md otherwise yaml generation on docs website doesn't work: docker#3924 (comment) Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Keep frontmatter for docker, dockerd and index markdown files. Also needs to move cli.md > docker.md before generation and then move it back because cli.md is needed for yaml generation on docs website: docker#3924 (comment) Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
b7d9555
to
8fbaac0
Compare
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Keep frontmatter for docker, dockerd and index markdown files. Also needs to move cli.md > docker.md before generation and then move it back because cli.md is needed for yaml generation on docs website: docker#3924 (comment) Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
8fbaac0
to
0bbcc52
Compare
Keep frontmatter for docker, dockerd and index markdown files. Also needs to move cli.md > docker.md before generation and then move it back because cli.md is needed for yaml generation on docs website: docker#3924 (comment) Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
0bbcc52
to
549166c
Compare
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!
I swapped the last two commits, so that we have the option of cherry-picking the anchor quote fixes
Arf, and of course then it wants the DCO to be fixed; let me do so |
Signed-off-by: Kevin Alvarez <crazy-max@users.noreply.github.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Keep frontmatter for docker, dockerd and index markdown files. Also needs to move cli.md > docker.md before generation and then move it back because cli.md is needed for yaml generation on docs website: docker#3924 (comment) Signed-off-by: Kevin Alvarez <crazy-max@users.noreply.github.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
549166c
to
79c9e52
Compare
See docker/cli#3924 and others
See docker/cli#3924 and others
follow-up docker/docs#16093 (comment)
- What I did
Set
"
as code delimiter for flags usage output to fix issue with docs generation.Also generates markdown in reference docs to be aligned as it seems we drifted a bit. Frontmatter section in md files doesn't seem to be used anymore so remove it as well.
And I found out that #2936 and #2024 break markdown tables (see last 2 commits).In follow-up we should also validate docs like it's done in buildx: https://github.com/docker/buildx/blob/88852e2330a200d495342e01783a922a5328d55c/hack/dockerfiles/docs.Dockerfile#L30-L43
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)