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

Update nexus commands.yml for docs gen #693

Merged
merged 10 commits into from
Oct 16, 2024
Merged

Conversation

prasek
Copy link
Contributor

@prasek prasek commented Oct 15, 2024

What was changed

See this branch for the combined approach with all PRs merged in.

Why?

To create CLI docs for Nexus.

Checklist

  1. How was this tested:

Ran go test ./...

Tested locally with:

go run ./cmd/temporal operator nexus endpoint create --name myendpoint --target-namespace my-target-namespace --target-task-queue my-handler-task-queue --description '## Sales Services
Workflow'\''s to support Customer-to-Order generation.

## other
stuff
'
  1. Any docs updates needed?

@prasek prasek marked this pull request as ready for review October 15, 2024 19:53
@prasek prasek changed the title update nexus commands.yml for docs gen Update nexus commands.yml for docs gen Oct 15, 2024
Signed-off-by: Phil Prasek <[email protected]>
temporalcli/commandsgen/commands.yml Outdated Show resolved Hide resolved
temporalcli/commandsgen/commands.yml Show resolved Hide resolved
@@ -3035,6 +3000,7 @@ option-sets:
env: TEMPORAL_CODEC_AUTH

- name: overlap-policy
description: Schedule commands
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem right that this is the same description for overlap-policy, schedule-id, and schedule-configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The option-set description is a new field added for option-set usages in the the combined approach, where it would show up as follows:

image

Until the the combined approach lands this field is not used.

Note the main purpose is to to see where divergent option descriptions are used (in commands or option-sets) in the generated cmd-options.mdx that collate on the various option descriptions under a given option name across command and option set usages.

After discussion with @yuandrew it's likely these would just be used for linting rules as we're moving away from cmd-options.mdx as it doesn't scale very well.

Given that will remove all these option-set descriptions for now and we can revisit later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's what you were going for. Sorry I was so confused. Thanks for the visual aid.

required: true

- name: nexus-endpoint-config
description: Nexus endpoint commands
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I see the benefit of option-set descriptions. They don't seem to add that much more information than what someone can glean from the option-set name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above, it was for cmd-options.mdx generation where we wanted to emit a description of option usage:

  • command usage or
  • option-set usage

wanted the usage site be human friendly vs. the option-name. another option would be to find all commands that use the option-set and add those so all option usages are commands, but think removing description for now is more expedient, so will do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. Suggest a quick case-sensitive scan to inspect endpoint lowercase/uppercase.

Signed-off-by: Phil Prasek <[email protected]>
@prasek
Copy link
Contributor Author

prasek commented Oct 16, 2024

feedback has been incorporated, think we're good to go.

@josh-berry @bergundy @fairlydurable

Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
Signed-off-by: Phil Prasek <[email protected]>
Copy link
Collaborator

@josh-berry josh-berry left a comment

Choose a reason for hiding this comment

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

LGTM apart from one comment. Feel free to merge once addressed, no need to re-review.

temporalcli/commandsgen/commands.yml Outdated Show resolved Hide resolved
Signed-off-by: Phil Prasek <[email protected]>
@josh-berry josh-berry merged commit cd53985 into temporalio:main Oct 16, 2024
7 checks passed
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.

5 participants