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

Accept profile argument in all subcommands #781

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Conversation

wfchandler
Copy link
Contributor

The profile argument is associated with the top-level oxide command, meaning it must be applied before a subcommand is registered. It would be convenient to be able to specify this arg at any point in the command, such as when repeating the same command against different profiles.

Set global to true for profile to allow this behavior.

@wfchandler wfchandler force-pushed the wc/profile-flag-global branch from 32d43f3 to 9ec9af2 Compare August 8, 2024 13:25
@wfchandler
Copy link
Contributor Author

Customer report from https://oxidecomputer.slack.com/archives/C06QW63CYPM/p1723121576190879?thread_ts=1723121438.371029&cid=C06QW63CYPM

The potential downside of this change is that it would prevent us from creating a --profile argument for a subcommand. None exist currently, and it seems like a bad idea to do so.

The `profile` argument is associated with the top-level `oxide` command,
meaning it must be applied before a subcommand is registered. It would
be convenient to be able to specify this arg at any point in the
command, such as when repeating the same command against different
profiles.

Set `global` to `true` for `profile` to allow this behavior.
@wfchandler wfchandler force-pushed the wc/profile-flag-global branch from 9ec9af2 to 4deda4f Compare August 8, 2024 13:33
@wfchandler wfchandler marked this pull request as ready for review August 8, 2024 13:44
@wfchandler wfchandler requested review from ahl and david-crespo August 8, 2024 13:44
@david-crespo
Copy link
Contributor

I am in favor of this — it came up on the original PR:

#727 (comment)

A small downside is that it adds noise to the docs by adding --profile to every command, but we can fix that by adding a global flag to each arg like I'm doing with required in #725, and then the docs site has to handle that. But I don't think we should block this change on that, we can do this and then fix that.

@david-crespo
Copy link
Contributor

I should note that I also have no problem hacking a fix into the docs site by hard coding a special case for the profile arg. Ideally temporarily, but it's so simple that it doesn't matter that much.

@augustuswm
Copy link
Contributor

augustuswm commented Aug 8, 2024

I think the upside of having it be allowed at any level is worth the extra text within the options flags outputs. Also agreed that it is a small downside to have it repeated on the docs, but certainly not worth blocking on.

I'm not sure how much custom work it would be to have global arguments that did not appear this way within clap.

@ahl ahl merged commit f60ae65 into main Aug 8, 2024
16 checks passed
@ahl ahl deleted the wc/profile-flag-global branch August 8, 2024 16:30
wfchandler added a commit that referenced this pull request Aug 28, 2024
With f60ae65 (Accept profile argument in all subcommands (#781),
2024-08-08) we set the `--profile` arg as global, allowing all
subcommands to inherit it. This is nice for usability, but adds a
considerable amount of clutter to our docs, as it now shows there for
each subcommand as well.

Filter out `--profile` when generating the docs.
wfchandler added a commit that referenced this pull request Aug 28, 2024
With f60ae65 (Accept profile argument in all subcommands (#781),
2024-08-08) we set `--profile` as a global arg, allowing all subcommands
to accept it. This is good for useability, but a bit of a mess in the
docs as it is now listed as an argument for all subcommands.

Removing it entirely from the docs hurts discoverability, but we want to
ensure it is clearly distinguished from the direct args for a
subcommand. Adding a subsection of the docs page with global args would
accomplish this.

Add a new `global` bool for `JsonArg` to indicate whether a given arg is
inherited.
wfchandler added a commit that referenced this pull request Aug 28, 2024
With f60ae65 (Accept profile argument in all subcommands (#781),
2024-08-08) we set `--profile` as a global arg, allowing all subcommands
to accept it. This is good for useability, but a bit of a mess in the
docs as it is now listed as an argument for all subcommands.

Removing it entirely from the docs hurts discoverability, but we want to
ensure it is clearly distinguished from the direct args for a
subcommand. Adding a subsection of the docs page with global args would
accomplish this.

Add a new `global` bool for `JsonArg` to indicate whether a given arg is
inherited.
david-crespo pushed a commit that referenced this pull request Aug 28, 2024
With f60ae65 (Accept profile argument in all subcommands (#781),
2024-08-08) we set `--profile` as a global arg, allowing all subcommands
to accept it. This is good for useability, but a bit of a mess in the
docs as it is now listed as an argument for all subcommands.

Removing it entirely from the docs hurts discoverability, but we want to
ensure it is clearly distinguished from the direct args for a
subcommand. Adding a subsection of the docs page with global args would
accomplish this.

Add a new `global` bool for `JsonArg` to indicate whether a given arg is
inherited.
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.

4 participants