-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix: remove list owners feature of info subcommand #14418
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we cannot simply 'fix' it
Does this mean owner information may not be added back when cargo info
hits stable? If it is the case, should we need a regression test ensuring this not happen again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will open a new issue to discuss this, as we need to consider including the download number in this PR. It may also be worth considering using a special API to fetch the owners list. However, we need to discuss this in the new issue.
When I say "fix it," I refer to not requiring authentication for this API. We cannot simply change that because, in our document, we state that it does require authentication. See owners list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood that. I was talking about the necessity of adding regression tests in this PR. If the feature requires more discussions and is not expected to be handled in a short term, maybe regression tests are worthy. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what kind of regression tests you are referring to. Do you mean to check there is no owners output or check for no credential operation output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is the latter one, I guess we can check it in the 'verbose' case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command won't fail under the reproduction step described in #14409
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I understand now. I was mistaken(stupid). I forgot that we can use that case. 😆 I will add a regression test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirm that with this .cargo/config.toml
[registry]
global-credential-providers = ["false"]
cargo info syn
succeeds to query crate information without requiring registry authentication.
@bors r+ Going to merge this as-is. Fine with or without regression test. Thank you! |
☀️ Test successful - checks-actions |
Update cargo 12 commits in ba8b39413c74d08494f94a7542fe79aa636e1661..8f40fc59fb0c8df91c97405785197f3c630304ea 2024-08-16 22:48:57 +0000 to 2024-08-21 22:37:06 +0000 - Tests rely on absence of RUST_BACKTRACE (rust-lang/cargo#14441) - fix: -Cmetadata includes whether extra rustflags is same as host (rust-lang/cargo#14432) - [mdman] Normalize newlines when rendering options (rust-lang/cargo#14428) - fix: doctest respects Cargo's color options (rust-lang/cargo#14425) - Be more permissive while packaging unpublishable crates. (rust-lang/cargo#14408) - fix: Limiting pre-release match semantics to use only on `OptVersionReq::Req` (rust-lang/cargo#14412) - test: add a regression test for Issue 14409 (rust-lang/cargo#14430) - chore: update label trigger for Command-info (rust-lang/cargo#14422) - doc: add lockfile-path unstable doc section (rust-lang/cargo#14423) - doc: update lockfile-path tracking issue (rust-lang/cargo#14424) - fix: remove list owners feature of info subcommand (rust-lang/cargo#14418) - Lockfile path tests (follow-up) (rust-lang/cargo#14417)
What does this PR try to resolve?
close #14409
Because this API requires authentication, the cargo info command should prompt the user to enter a password every time it is used(for those with a hardware token, a hardware key).
Since this commit 0c25226, authentication is required for this API.
So we cannot simply 'fix' it. So we decided to delete this feature first.
How should we test and review this PR?
Check the updated test snapshots.
Additional information
None