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

feat(docs): add UsageText to docs output for markdown and man page generation #1171

Merged
merged 3 commits into from May 21, 2021

Conversation

clok
Copy link
Contributor

@clok clok commented Aug 6, 2020

What type of PR is this?

(REQUIRED)

  • bug
  • cleanup
  • documentation
  • feature

What this PR does / why we need it:

(REQUIRED)

As a tools developer, I find the UsageText field to be useful in providing details on what a command does and how to use it. A good example is with gwsm

For example, the command gwsm diff has as detailed description that helps the user know how to use the command.

$ gwsm diff --help
NAME:
   gwsm diff - View diff of local vs. namespace

USAGE:

View the diff of the local environment against a given command running on a pod within a namespace.

This will retrieve the stored secrets within AWS Secrets Manager and map them via
the secrets.yml file used by the 'summon' CLI tool to generate the current state of
Environment Variables for a given stage.

The AWS Secrets Manager names are assumed to be stored as '<SECRETS_GROUP>_NAME'
in the ConfigMap. Example: 'RDS_SECRET_NAME: rds/staging/service-yolo'

From the root of the service, the required files are typically found below:

The path to the configmap.yaml file is within the kubernetes deployment.
This is typically .kube/<stage>/05-configmap.yaml

The path to the secrets.yml is typically .docker/secrets.yaml

It will then grab current environment for a specific process running within a Pod in a given Namespace.

This is achieved by inspecting the /proc/<PID>/environ for the given process. This method uses
'/bin/bash -c' as the base command to perform the PID inspection via 'ps faux'.

The 'filter-prefix' flag will exclude any values that start with the flagged prefixes from display.

The 'exclude' flag will exclude any values where the KEY matches exactly from display.


OPTIONS:
   --secrets value, -s value        Path to secrets.yml (default: ".docker/secrets.yml")
   --configmap value, -c value      Path to configmap.yaml
   --namespace value, -n value      Kube Namespace list Pods from
   --cmd value                      Command to inspect (default: "node")
   --filter-prefix value, -f value  List of prefixes (csv) used to filter values from display. Set to "" to remove any filters. (default: "npm_,KUBERNETES_,API_PORT")
   --exclude value                  List (csv) of specific env vars to exclude values from display. Set to "" to remove any exclusions. (default: "PATH,SHLVL,HOSTNAME")
   --secret-suffix value            Suffix used to find ENV variables that denote the Secret Manager Secrets to lookup (default: "_NAME")
   --help, -h                       show help (default: false)

I would like for the docs generation (ToMarkdown and ToMan) to include this help text in the docs.

See the before and after here:

before after
markdown https://gist.github.com/clok/ca1452874e372b02fe597f8ad65ca7f2 https://gist.github.com/clok/fe5e00c973575778f8e548263b5dba44
man https://gist.github.com/clok/4ad90b19868709c86612ff76ee0e1350 https://gist.github.com/clok/a810114a454beee90d5134e9c8c533e8

Which issue(s) this PR fixes:

(REQUIRED)

N/A - New feature

Testing

Updated a local copy of an existing tool (gwsm) with the delta applied and generated before/after docs.

See the before and after here:

before after
markdown https://gist.github.com/clok/ca1452874e372b02fe597f8ad65ca7f2 https://gist.github.com/clok/fe5e00c973575778f8e548263b5dba44
man https://gist.github.com/clok/4ad90b19868709c86612ff76ee0e1350 https://gist.github.com/clok/a810114a454beee90d5134e9c8c533e8
  • Updated test suite
  • Generated local examples

Release Notes

(REQUIRED)

Add UsageText to docs output for markdown and man page generation

@clok clok requested a review from a team as a code owner August 6, 2020 16:51
@clok clok requested review from rliebz and coilysiren and removed request for a team August 6, 2020 16:51
paulojblack
paulojblack previously approved these changes Aug 6, 2020
@clok
Copy link
Contributor Author

clok commented Nov 2, 2020

@rliebz and @lynncyrin any chance that this can get merged?

Copy link
Member

@rliebz rliebz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, and sorry for the delay here. Got a few comments for you, but overall this looks great!

docs.go Outdated
usageText := ""
if command.UsageText != "" {
// Remove leading and trailing newlines
preparedUsageText := strings.TrimSuffix(command.UsageText, "\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to intentionally trim exactly up to one leading newline and one trailing newline? Or is the goal to trim all leading/trailing newlines?

For trimming all newlines from either side:

preparedUsageText := strings.Trim(command.UsageText, "\n")

For trimming all whitespace from either end:

preparedUsageText := strings.TrimSpace(command.UsageText)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preparedUsageText := strings.Trim(command.UsageText, "\n")

That is what I was looking for. Thank you for the suggestion! It was intended to remove all leading and trailing newlines.

docs.go Outdated
// Format multi-line string as a code block
usageText = fmt.Sprintf("```\n%s\n```\n", preparedUsageText)
} else {
// Style a single line as a note
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 curious—why use a block-quote for a one-line usage text? I'm not sure I understand why the style would be different for a one-liner versus a multi-liner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a styling opinion more than anything. The use case I was considering was a very long one-line comment. Say 200+ characters in length. Within markdown there is no styling for the wrapping rules on a pre-formatted text block. This is particularly pronounced on Github where the CSS rules for a text block are quite narrow. Take the case below as an example.

This will retrieve the stored secrets within AWS Secrets Manager and map them via the secrets.yml file used by the 'summon' CLI tool to generate the current state of Environment Variables for a given stage.

versus

This will retrieve the stored secrets within AWS Secrets Manager and map them via the secrets.yml file used by the 'summon' CLI tool to generate the current state of Environment Variables for a given stage.

With a > note syntax, the markdown render will wrap the text accordingly. With the pre-formatted text block, it scrolls off screen making it difficult to read.

I dabbled with using > across many lines, but there were quite a few edge cases that made predicting the desired markdown format unruly.

docs.go Outdated Show resolved Hide resolved
docs.go Outdated Show resolved Hide resolved
docs.go Outdated Show resolved Hide resolved
docs.go Outdated Show resolved Hide resolved
docs.go Outdated Show resolved Hide resolved
docs.go Show resolved Hide resolved
@clok
Copy link
Contributor Author

clok commented Nov 14, 2020

Thank you very much for the feedback and suggestions. I will get to updating tests and responding to the feedback. I had hoped to get to it last week, but life and work kept pulling me away. Sorry for the delay!

@clok
Copy link
Contributor Author

clok commented Nov 16, 2020

@rliebz Rebased with current master and updated with your feedback. Thank you again for the suggestions and comments. I noticed that the github actions are failing, but I can't tell if it is due to the changes I pushed. I am happy to address any issues that I can. Just let me know.

@clok clok requested a review from rliebz November 16, 2020 04:44
@stale
Copy link

stale bot commented Feb 14, 2021

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

@stale stale bot added the status/stale stale due to the age of it's last update label Feb 14, 2021
@stale
Copy link

stale bot commented Feb 26, 2021

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

@stale stale bot removed the status/stale stale due to the age of it's last update label Feb 26, 2021
@clok
Copy link
Contributor Author

clok commented Feb 26, 2021

I put together an extension of sorts that accomplishes this feature (and more). https://github.com/clok/cdocs

I would really like to put these features in the mainline as I feel they are useful for all cli users. The plan was to get this initial work through the process and then extend from there.

I have updated the code, fixed the linting errors. Waiting for a review from maintainers. Thanks!

@coilysiren coilysiren requested review from a team and removed request for coilysiren and rliebz April 23, 2021 19:44
@codefromthecrypt
Copy link

any chance this can be reconsidered? It seems a pain to have to do external markdown generation to accomplish the same meanwhile, even if there's an add-on

Copy link
Member

@rliebz rliebz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry again for the delays here! This looks great, thanks for the contribution!

@rliebz rliebz merged commit 581b769 into urfave:master May 21, 2021
@clok
Copy link
Contributor Author

clok commented May 21, 2021

Awesome! Happy to contribute.

@codefromthecrypt
Copy link

I think this may have drifted the -h output. As you'll notice, when multiple lines are present, it doesn't make a consistent indent.

USAGE:
   The '<version>' is from the "versions" command and installed if necessary.
The '<args>' are interpreted by Envoy.
The Envoy working directory is archived as $GETENVOY_HOME/runs/$epochtime.tar.gz upon termination.

Example:
$ getenvoy run 1.18.3 --config-path ./bootstrap.yaml

@codefromthecrypt
Copy link

the above was from this..

		Usage:     "Download and install a <version> of Envoy",
		ArgsUsage: "<version>",
		UsageText: fmt.Sprintf(`The '<version>' is from the "versions" command.
The Envoy <version> will be installed into $GETENVOY_HOME/versions/<version>

Example:
$ getenvoy install %s`, version.LastKnownEnvoy),

@rliebz
Copy link
Member

rliebz commented Jun 1, 2021

@codefromthecrypt Would you mind opening an issue for that? If there's a bug in the behavior here, it's easier to keep track of if we have an issue with the expected behavior and steps to reproduce.

@codefromthecrypt
Copy link

@rliebz sure #1278

@rliebz
Copy link
Member

rliebz commented Jun 1, 2021

Thanks!

@clok
Copy link
Contributor Author

clok commented Jun 2, 2021

Nice catch on the regression. I'm sorry about that unintended change. I'll take a look at the issue and if it hasn't been addressed yet, I'll work on a fix for it.

@clok clok deleted the feat/docs-usage-text branch June 2, 2021 17:18
@clok
Copy link
Contributor Author

clok commented Jun 2, 2021

Got a PR up (#1279) to address the issue raised by @codefromthecrypt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants