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

Support cargo owner add #11879

Closed
wants to merge 42 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
786e0b6
Support cargo owner add
heisen-li Mar 23, 2023
9bef78d
Merge remote-tracking branch 'upstream/master' into owner
heisen-li Mar 23, 2023
5862698
Delete res.txt
heisen-li Mar 23, 2023
8914b1a
Update registry.rs
heisen-li Mar 23, 2023
6573a9e
Merge branch 'rust-lang:master' into owner
heisen-li Apr 2, 2023
baf3020
Added test cases and modified document descriptions.
heisen-li Apr 2, 2023
0ae910a
Merge remote-tracking branch 'upstream/owner' into owner
heisen-li Apr 2, 2023
efaf17b
fix doc
heisen-li Apr 2, 2023
6037751
Merge branch 'rust-lang:master' into owner
heisen-li Apr 3, 2023
3a99929
fix some problem
heisen-li Apr 4, 2023
1423140
Merge remote-tracking branch 'upstream/owner' into owner
heisen-li Apr 4, 2023
a2a59ea
Revised based on review comments
heisen-li Apr 4, 2023
08be20f
Use enumerate and move parameters to subcommands
heisen-li Apr 10, 2023
e038e62
fix
heisen-li Apr 16, 2023
21afbcf
fix
heisen-li Apr 16, 2023
1ef930f
Remove some test cases.
heisen-li Apr 16, 2023
a8b46be
update
heisen-li Nov 2, 2023
33ca2b8
update
heisen-li Nov 2, 2023
daaa8c1
delete file
heisen-li Nov 2, 2023
fc7987e
update tests
heisen-li Nov 2, 2023
5a988dd
Merge remote-tracking branch 'origin/owner' into owner
heisen-li Nov 2, 2023
3fc6b53
fix some error
heisen-li Nov 2, 2023
1dc82ae
add some test & fix some error
heisen-li Nov 3, 2023
415bbf6
fix conflicts
heisen-li Nov 13, 2023
60ebb2a
add doc man
heisen-li Nov 20, 2023
3fa434d
modify
heisen-li Nov 21, 2023
3678dff
Merge branch 'rust-lang:master' into owner
heisen-li Nov 23, 2023
7acc02f
fix test and some
heisen-li Nov 25, 2023
bd2f6ef
Merge branch 'owner' of https://github.com/heisen-li/cargo into owner
heisen-li Nov 25, 2023
13fccb2
fix test
heisen-li Nov 25, 2023
bf304aa
modify the code by comments
heisen-li Dec 12, 2023
97d2596
resolve conflict
heisen-li Dec 12, 2023
0987b72
resolve conflict
heisen-li Dec 12, 2023
05c1653
resolve conflict
heisen-li Dec 12, 2023
1bc57c7
Merge branch 'owner' of https://github.com/heisen-li/cargo into owner
heisen-li Dec 12, 2023
b722762
fix failed test
heisen-li Dec 12, 2023
01701f7
fix failed tests
heisen-li Dec 12, 2023
9f7e9bd
fix failed tests
heisen-li Dec 12, 2023
b01c084
Merge branch 'owner' of https://github.com/heisen-li/cargo into owner
heisen-li Dec 12, 2023
3e1914e
fix tests
heisen-li Dec 12, 2023
6d2b554
zsh completions
heisen-li Dec 20, 2023
48b5429
fix conflict
heisen-li Jan 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 73 additions & 25 deletions src/bin/cargo/commands/owner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
epage marked this conversation as resolved.
Show resolved Hide resolved
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 mixed about whether cargo owner should show --help or should default to cargo owner list

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

  • Clearer meaning
  • ownername can use num_args(1..)
  • Might be less confusing than a straight parameter order swap

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

  1. Currently, it is supported to use .value_delimiter(',') to add or delete multiple owners at one time;
  2. If ownername and cratename are optional, cargo owner add/remove will have no meaning;

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"))
Expand All @@ -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(())
}
117 changes: 73 additions & 44 deletions src/cargo/ops/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ use crate::util::{truncate_with_ellipsis, IntoUrl};
use crate::util::{Progress, ProgressStyle};
use crate::{drop_print, drop_println, version};

use crate::util::command_prelude::ArgMatches;

/// Registry settings loaded from config files.
///
/// This is loaded based on the `--registry` flag and the config settings.
Expand Down Expand Up @@ -972,18 +974,21 @@ pub fn registry_logout(config: &Config, reg: Option<&str>) -> CargoResult<()> {
}

pub struct OwnersOptions {
pub krate: Option<String>,
pub token: Option<Secret<String>>,
pub index: Option<String>,
pub to_add: Option<Vec<String>>,
pub to_remove: Option<Vec<String>>,
pub list: bool,
pub subcommand: Option<String>,
heisen-li marked this conversation as resolved.
Show resolved Hide resolved
pub subcommand_arg: Option<ArgMatches>,
heisen-li marked this conversation as resolved.
Show resolved Hide resolved
pub registry: Option<String>,
}

pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> {
let name = match opts.krate {
Some(ref name) => name.clone(),
let name = match opts
.subcommand_arg
.as_ref()
.unwrap()
.get_one::<String>("cratename")
{
Some(ref name) => name.clone().to_string(),
None => {
let manifest_path = find_root_manifest_for_wd(config.cwd())?;
let ws = Workspace::new(&manifest_path, config)?;
Expand All @@ -1002,49 +1007,73 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> {
Some(mutation),
)?;

if let Some(ref v) = opts.to_add {
let v = v.iter().map(|s| &s[..]).collect::<Vec<_>>();
let msg = registry.add_owners(&name, &v).with_context(|| {
format!(
"failed to invite owners to crate `{}` on registry at {}",
name,
registry.host()
)
})?;
match opts.subcommand.as_ref().map(|s| s.as_str()) {
Some("add") => {
let v = opts
.subcommand_arg
.as_ref()
.unwrap()
.get_many::<String>("ownername")
.map(|s| s.collect::<Vec<_>>())
.and_then(|t| Some(t.iter().map(|s| s.as_str()).collect::<Vec<_>>()))
.unwrap();
let msg = registry.add_owners(&name, &v).with_context(|| {
format!(
"failed to invite owners to crate `{}` on registry at {}",
name,
registry.host()
)
})?;

config.shell().status("Owner", msg)?;
}
config.shell().status("Owner", msg)?;
}

if let Some(ref v) = opts.to_remove {
let v = v.iter().map(|s| &s[..]).collect::<Vec<_>>();
config
.shell()
.status("Owner", format!("removing {:?} from crate {}", v, name))?;
registry.remove_owners(&name, &v).with_context(|| {
format!(
"failed to remove owners from crate `{}` on registry at {}",
name,
registry.host()
)
})?;
}
Some("remove") => {
let v = opts
.subcommand_arg
.as_ref()
.unwrap()
.get_many::<String>("ownername")
.map(|s| s.collect::<Vec<_>>())
.and_then(|t| Some(t.iter().map(|s| s.as_str()).collect::<Vec<_>>()))
.unwrap();
config
.shell()
.status("Owner", format!("removing {:?} from crate {}", v, name))?;
registry.remove_owners(&name, &v).with_context(|| {
format!(
"failed to remove owners from crate `{}` on registry at {}",
name,
registry.host()
)
})?;
}

if opts.list {
let owners = registry.list_owners(&name).with_context(|| {
format!(
"failed to list owners of crate `{}` on registry at {}",
name,
registry.host()
)
})?;
for owner in owners.iter() {
drop_print!(config, "{}", owner.login);
match (owner.name.as_ref(), owner.email.as_ref()) {
(Some(name), Some(email)) => drop_println!(config, " ({} <{}>)", name, email),
(Some(s), None) | (None, Some(s)) => drop_println!(config, " ({})", s),
(None, None) => drop_println!(config),
Some("list") => {
let owners = registry.list_owners(&name).with_context(|| {
format!(
"failed to list owners of crate `{}` on registry at {}",
name,
registry.host()
)
})?;
for owner in owners.iter() {
drop_print!(config, "{}", owner.login);
match (owner.name.as_ref(), owner.email.as_ref()) {
(Some(name), Some(email)) => drop_println!(config, " ({} <{}>)", name, email),
(Some(s), None) | (None, Some(s)) => drop_println!(config, " ({})", s),
(None, None) => drop_println!(config),
}
}
}

_ => {
anyhow::bail!(
"
You have entered an incorrect subcommand. \
Run the `--help` command to obtain more information."
);
}
}

Ok(())
Expand Down
10 changes: 5 additions & 5 deletions src/doc/src/reference/publishing.md
Original file line number Diff line number Diff line change
Expand Up @@ -169,15 +169,15 @@ may change over time! The owner of a crate is the only person allowed to publish
new versions of the crate, but an owner may designate additional owners.

```console
$ cargo owner --add github-handle
$ cargo owner --remove github-handle
$ cargo owner --add github:rust-lang:owners
$ cargo owner --remove github:rust-lang:owners
$ cargo owner add github-handle
epage marked this conversation as resolved.
Show resolved Hide resolved
$ cargo owner remove github-handle
$ cargo owner add github:rust-lang:owners
$ cargo owner remove github:rust-lang:owners
```

The owner IDs given to these commands must be GitHub user names or GitHub teams.

If a user name is given to `--add`, that user is invited as a “named” owner, with
If a user name is given to `add`, that user is invited as a “named” owner, with
full rights to the crate. In addition to being able to publish or yank versions
of the crate, they have the ability to add or remove owners, *including* the
owner that made *them* an owner. Needless to say, you shouldn’t make people you
Expand Down
25 changes: 17 additions & 8 deletions tests/testsuite/alt_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,7 @@ fn no_api() {
.with_stderr_contains(&err)
.run();

p.cargo("owner --registry alternative --list")
p.cargo("owner --registry alternative list")
heisen-li marked this conversation as resolved.
Show resolved Hide resolved
.with_status(101)
.with_stderr_contains(&err)
.run();
Expand Down Expand Up @@ -1362,20 +1362,29 @@ fn both_index_and_registry() {
#[cargo_test]
fn both_index_and_default() {
let p = project().file("src/lib.rs", "").build();
for cmd in &[
"publish",
"owner",
"search",
"yank --version 1.0.0",
"install foo",
] {
for cmd in &["publish", "search", "yank --version 1.0.0", "install foo"] {
p.cargo(cmd)
.env("CARGO_REGISTRY_DEFAULT", "undefined")
.arg(format!("--index=index_url"))
.with_status(101)
.with_stderr("[ERROR] invalid url `index_url`: relative URL without a base")
.run();
}
p.cargo("owner --index=index_url add someone")
.env("CARGO_REGISTRY_DEFAULT", "undefined")
.with_status(101)
.with_stderr("[ERROR] invalid url `index_url`: relative URL without a base")
.run();
heisen-li marked this conversation as resolved.
Show resolved Hide resolved
}

#[cargo_test]
fn owner_index_and_default() {
let p = project().file("src/lib.rs", "").build();
p.cargo("owner --index=index_url add someone")
.env("CARGO_REGISTRY_DEFAULT", "undefined")
.with_status(101)
.with_stderr("[ERROR] invalid url `index_url`: relative URL without a base")
.run();
}
heisen-li marked this conversation as resolved.
Show resolved Hide resolved

#[cargo_test]
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/credential_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ fn yank() {
fn owner() {
let (p, _t) = get_token_test();

p.cargo("owner --add username --registry alternative -Z credential-process")
p.cargo("owner --registry alternative -Z credential-process add username")
.masquerade_as_nightly_cargo(&["credential-process"])
.with_stderr(
"\
Expand Down
Loading