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

Package Rename bug fix #271

Merged
merged 1 commit into from
Aug 10, 2024
Merged

Package Rename bug fix #271

merged 1 commit into from
Aug 10, 2024

Conversation

confused-Techie
Copy link
Member

Requirements

  • Filling out the template is required.

  • All new code requires tests to ensure against regressions.

    • However, if your PR contains zero code changes, feel free to select the checkmark below to indicate so.
  • Have you ran tests against this code?

  • This PR contains zero code changes.

Description of the Change

As helpfully discovered by @savetheclocktower we currently, mistakenly check for the truthy return of db.packageNameAvailability to fail on a package rename, when we need to check it's falsey return.

@savetheclocktower
Copy link

I just observed the effects — you found the bug :)

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

I assume what this code means is, as of the fix, is that if new name is not available (!isAvailable.ok) we print an error, otherwise we (the backend) continue on to publishing a package and doing all that other stuff.

Seems legit! I can see why that being inverted before could count as a bug.

(Mostly piggy-backing off of confused and savetheclocktower saying this seems to be right, TBH. But this seems like a sensible fix to my own judgment as well. Just, I'm not that familiar with the backend code, is all.)

@DeeDeeG
Copy link
Member

DeeDeeG commented Aug 8, 2024

If it's alright with everybody, I'll wait on this to land before approving pulsar-edit/ppm#135, since as it is right now I can't end-to-end test that that ppm fix is working, whereas this one appears if I'm following conversation correctly to be the real root-cause issue, and the ppm one is a more superficial one (or at least a client bug rather than infra bug).

Maybe that rounds down to my having one reason: I'd like to be able to test better, heh.

@confused-Techie
Copy link
Member Author

Thanks a ton for the reviews @savetheclocktower & @DeeDeeG I'll get this one merged and deployed

@confused-Techie confused-Techie merged commit 2473f54 into main Aug 10, 2024
6 checks passed
@confused-Techie confused-Techie deleted the rename-bug-fix branch August 10, 2024 07:27
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.

3 participants