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

Add PATCH /crates/:crate/:version route #9423

Merged
merged 5 commits into from
Sep 30, 2024

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Sep 10, 2024

ref #9193

This pull request introduces a new patch API for yanking and unyanking crates with a message.

The logic is very similar to the current yank and unyank API, but it supports setting a yank message.

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 98.49246% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.11%. Comparing base (d02745c) to head (45ec3fb).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/controllers/version/metadata.rs 97.02% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9423      +/-   ##
==========================================
+ Coverage   89.05%   89.11%   +0.06%     
==========================================
  Files         285      285              
  Lines       28757    28920     +163     
==========================================
+ Hits        25610    25773     +163     
  Misses       3147     3147              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-update-patch branch 4 times, most recently from 4f51afb to 629c370 Compare September 11, 2024 13:56
Rustin170506

This comment was marked as outdated.

@Rustin170506 Rustin170506 marked this pull request as ready for review September 11, 2024 14:15
@Rustin170506 Rustin170506 requested a review from a team September 11, 2024 14:15
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

🔢 Self-check (PR reviewed by myself and ready for feedback.)

@Rustin170506 Rustin170506 force-pushed the rustin-patch-update-patch branch 3 times, most recently from fb5c1f2 to 30b5ddb Compare September 11, 2024 14:28
@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Sep 11, 2024
Copy link
Contributor

@eth3lbert eth3lbert left a comment

Choose a reason for hiding this comment

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

I've left some suggestions before a full review.

src/controllers/version/metadata.rs Outdated Show resolved Hide resolved
src/controllers/version/metadata.rs Show resolved Hide resolved
src/controllers/version/metadata.rs Outdated Show resolved Hide resolved
@Turbo87
Copy link
Member

Turbo87 commented Sep 12, 2024

I've rebased this on top of #9437 to get that first part out of the way and make the PR a bit easier to review :)

src/tests/krate/yanking.rs Outdated Show resolved Hide resolved
src/tests/krate/yanking.rs Outdated Show resolved Hide resolved
src/controllers/version/metadata.rs Show resolved Hide resolved
@Rustin170506 Rustin170506 marked this pull request as draft September 13, 2024 02:25
@Rustin170506 Rustin170506 force-pushed the rustin-patch-update-patch branch 2 times, most recently from f00807f to 3e33515 Compare September 13, 2024 15:17
@Rustin170506 Rustin170506 changed the title Add PATCH /crates/:crate/:version route WIP: Add PATCH /crates/:crate/:version route Sep 13, 2024
@Rustin170506 Rustin170506 force-pushed the rustin-patch-update-patch branch 3 times, most recently from a9e1768 to 5f8fbb9 Compare September 18, 2024 14:30
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

🔢 Self-check (PR reviewed by myself and ready for feedback.)

@Rustin170506 Rustin170506 changed the title WIP: Add PATCH /crates/:crate/:version route Add PATCH /crates/:crate/:version route Sep 18, 2024
@Rustin170506 Rustin170506 marked this pull request as ready for review September 18, 2024 14:45
@bors
Copy link
Contributor

bors commented Sep 25, 2024

☔ The latest upstream changes (presumably #9505) made this pull request unmergeable. Please resolve the merge conflicts.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-update-patch branch 5 times, most recently from ab6eb47 to bdc7968 Compare September 25, 2024 15:25
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

🔢 Self-check (PR reviewed by myself and ready for feedback.)

@bors
Copy link
Contributor

bors commented Sep 27, 2024

☔ The latest upstream changes (presumably 60d3cfe) made this pull request unmergeable. Please resolve the merge conflicts.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-update-patch branch 3 times, most recently from 23adba8 to 92a4793 Compare September 27, 2024 13:26
Rustin170506 and others added 5 commits September 30, 2024 13:38
Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>
This was using the previous `yanked` state for the admin log message, instead of the new state.
Comment on lines 277 to 288
// Attempt to set yank message on unyanked version (should fail)
token
.update_yank_status("patchable", "1.0.0", None, Some("Invalid message"))
.await
.status()
.is_client_error();
// Attempt to unyank with message (should fail)
token
.update_yank_status("patchable", "1.0.0", Some(false), Some("Invalid message"))
.await
.status()
.is_client_error();
Copy link
Member

Choose a reason for hiding this comment

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

these calls just return bool, but they are missing assertions. probably best to assert against a specific status code though.

src/controllers/version/metadata.rs Show resolved Hide resolved

if Handle::current().block_on(user.rights(state, &owners))? < Rights::Publish {
if user.is_admin {
let action = if version.yanked {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be using the yanked variable from below 🤔

@Turbo87
Copy link
Member

Turbo87 commented Sep 30, 2024

I rebased the branch and fixed the above two comments. I think this is good to go now :)

@Turbo87 Turbo87 merged commit 7cae863 into rust-lang:main Sep 30, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants