Skip to content
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

Merged
merged 5 commits into from Jan 6, 2023

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Dec 18, 2022

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)

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2022

Codecov Report

Merging #3924 (b7d9555) into master (99023cb) will decrease coverage by 0.00%.
The diff coverage is 73.21%.

❗ Current head b7d9555 differs from pull request most recent head 8fbaac0. Consider uploading reports for the commit 8fbaac0 to get more accurate results

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              

@crazy-max
Copy link
Member Author

@thaJeztah I wonder if https://github.com/docker/cli/blob/master/docs/reference/commandline/dockerd.md should not be in moby repo instead?

cli/flags/options.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

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 😅 😓).

Copy link
Member

@thaJeztah thaJeztah left a 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 😄 )

scripts/docs/generate-yaml.sh Outdated Show resolved Hide resolved
docs/reference/commandline/config_inspect.md Outdated Show resolved Hide resolved
cli/command/container/update.go Outdated Show resolved Hide resolved
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")`)
Copy link
Member

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)

/cc @dvdksn @vvoland @usha-mandya

Copy link
Contributor

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.

Copy link
Contributor

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")`)

@thaJeztah
Copy link
Member

Looks like an e2e test needs updating;

=== FAIL: e2e/stack TestStackDeployHelp (0.05s)
    help_test.go:13: assertion failed: 
        --- expected
        +++ actual
        @@ -12,6 +12,6 @@
         ······--prune··················Prune·services·that·are·no·longer·referenced
         ······--resolve-image·string···Query·the·registry·to·resolve·image·digest
        -·······························and·supported·platforms
        -·······························("always",·"changed",·"never")·(default·"always")
        +·······························and·supported·platforms·("always",
        +·······························"changed",·"never")·(default·"always")
         ······--with-registry-auth·····Send·registry·authentication·details·to
         ·······························Swarm·agents
        
        
        You can run 'go test . -update' to automatically update testdata/stack-deploy-help.golden to the new expected value.'

thaJeztah pushed a commit to thaJeztah/cli that referenced this pull request Jan 6, 2023
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>
crazy-max added a commit to crazy-max/docker-cli that referenced this pull request Jan 6, 2023
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>
thaJeztah pushed a commit to thaJeztah/cli that referenced this pull request Jan 6, 2023
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>
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/cli.md Outdated Show resolved Hide resolved
docs/reference/commandline/docker.md Outdated Show resolved Hide resolved
thaJeztah pushed a commit to thaJeztah/cli that referenced this pull request Jan 6, 2023
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>
thaJeztah pushed a commit to thaJeztah/cli that referenced this pull request Jan 6, 2023
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>
crazy-max added a commit to crazy-max/docker-cli that referenced this pull request Jan 6, 2023
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>
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>
crazy-max added a commit to crazy-max/docker-cli that referenced this pull request Jan 6, 2023
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>
thaJeztah pushed a commit to thaJeztah/cli that referenced this pull request Jan 6, 2023
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>
Copy link
Member

@thaJeztah thaJeztah left a 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

@thaJeztah
Copy link
Member

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants