-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Support cargo owner add
#11879
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
Support cargo owner add
#11879
Changes from 9 commits
786e0b6
9bef78d
5862698
8914b1a
6573a9e
baf3020
0ae910a
efaf17b
6037751
3a99929
1423140
a2a59ea
08be20f
e038e62
21afbcf
1ef930f
a8b46be
33ca2b8
daaa8c1
fc7987e
5a988dd
3fc6b53
1dc82ae
415bbf6
60ebb2a
3fa434d
3678dff
7acc02f
bd2f6ef
13fccb2
bf304aa
97d2596
0987b72
05c1653
1bc57c7
b722762
01701f7
9f7e9bd
b01c084
3e1914e
6d2b554
48b5429
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,25 +6,68 @@ use cargo::util::auth::Secret; | |
pub fn cli() -> Command { | ||
subcommand("owner") | ||
.about("Manage the owners of a crate on the registry") | ||
.arg_quiet() | ||
.arg(Arg::new("crate").action(ArgAction::Set)) | ||
.arg( | ||
multi_opt( | ||
"add", | ||
"LOGIN", | ||
"Name of a user or team to invite as an owner", | ||
) | ||
.short('a'), | ||
) | ||
.arg( | ||
multi_opt( | ||
"remove", | ||
"LOGIN", | ||
"Name of a user or team to remove as an owner", | ||
) | ||
.short('r'), | ||
.arg_required_else_help(true) | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm mixed about whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think any results should be shown without explicit user action. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A lot of CLIs default a command to list. However, since this is a network operation, I can see foregoing that. As this is an error, we can change it in the future. |
||
.override_usage( | ||
"\ | ||
cargo owner [OPTIONS] add OWNER_NAME <CRATE_NAME> | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cargo owner [OPTIONS] remove OWNER_NAME <CRATE_NAME> | ||
cargo owner [OPTIONS] list <CRATE_NAME>", | ||
heisen-li marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
.arg(flag("list", "List owners of a crate").short('l')) | ||
.arg_quiet() | ||
.subcommands([ | ||
Command::new("add") | ||
.about("Name of a user or team to invite as an owner") | ||
.override_usage( | ||
"\ | ||
cargo owner [OPTIONS] add [OWNER_NAME] <CRATE_NAME>", | ||
) | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.arg_quiet() | ||
.args([ | ||
Arg::new("ownername") | ||
heisen-li marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.action(ArgAction::Set) | ||
heisen-li marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.required(true) | ||
.num_args(1) | ||
heisen-li marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.value_delimiter(',') | ||
.value_name("OWNER_NAME"), | ||
Arg::new("cratename") | ||
heisen-li marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know ehuss mentioned making this a positional but I wonder if it should be an option instead, especially now that its optional. I see positionals like function parameters (in languages with named parameters): if the meaning isn't obvious by the position then it should be a named parameter. Benefits
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean to change ownername and crate name to options? I want to discuss:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking owners be a required positional and package name be an option that gets defaulted to the one for the parent directory. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now, crate name is an option. |
||
.action(ArgAction::Set) | ||
.num_args(1) | ||
.value_name("CRATE_NAME"), | ||
]), | ||
Command::new("remove") | ||
.about("Name of a user or team to remove as an owner") | ||
.override_usage( | ||
"\ | ||
cargo owner [OPTIONS] remove [OWNER_NAME] <CRATE_NAME>", | ||
) | ||
.arg_quiet() | ||
.args([ | ||
Arg::new("ownername") | ||
.action(ArgAction::Set) | ||
.required(true) | ||
.num_args(1) | ||
.value_delimiter(',') | ||
.value_name("OWNER_NAME"), | ||
Arg::new("cratename") | ||
.action(ArgAction::Set) | ||
.num_args(1) | ||
.value_name("CRATE_NAME"), | ||
]), | ||
Command::new("list") | ||
.about("List owners of a crate") | ||
.override_usage( | ||
"\ | ||
cargo owner [OPTIONS] list <CRATE_NAME>", | ||
) | ||
.arg_quiet() | ||
.arg( | ||
Arg::new("cratename") | ||
.action(ArgAction::Set) | ||
.num_args(1) | ||
.value_name("CRATE_NAME"), | ||
), | ||
]) | ||
.arg(opt("index", "Registry index to modify owners for").value_name("INDEX")) | ||
.arg(opt("token", "API token to use when authenticating").value_name("TOKEN")) | ||
.arg(opt("registry", "Registry to use").value_name("REGISTRY")) | ||
|
@@ -33,19 +76,24 @@ pub fn cli() -> Command { | |
|
||
pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { | ||
let registry = args.registry(config)?; | ||
|
||
let Some((sc, arg)) = args.subcommand() else { | ||
return Err(CliError::new( | ||
anyhow::format_err!( | ||
"you need to specify the subcommands to be operated: add, remove or list." | ||
), | ||
101, | ||
)); | ||
}; | ||
heisen-li marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
let opts = OwnersOptions { | ||
krate: args.get_one::<String>("crate").cloned(), | ||
token: args.get_one::<String>("token").cloned().map(Secret::from), | ||
index: args.get_one::<String>("index").cloned(), | ||
to_add: args | ||
.get_many::<String>("add") | ||
.map(|xs| xs.cloned().collect()), | ||
to_remove: args | ||
.get_many::<String>("remove") | ||
.map(|xs| xs.cloned().collect()), | ||
list: args.flag("list"), | ||
subcommand: Some(sc.to_owned()), | ||
subcommand_arg: Some(arg.clone()), | ||
registry, | ||
}; | ||
|
||
ops::modify_owners(config, &opts)?; | ||
Ok(()) | ||
} |
Uh oh!
There was an error while loading. Please reload this page.