-
Notifications
You must be signed in to change notification settings - Fork 1k
Add ability for maintainers to provide reason for yanking a release #7916
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
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.
I'm trying to think about where to add tests...the process of adding a reason and making sure it shows up feels like a functional test, of which there are rather few. This new feature didn't introduce new methods/functions, so maybe I'm also overthinking it. Coverage remains at 100%, but I wouldn't mind exercising the new feature more explicitly.
@di Let me know if you have any guidance on the above notes! Is it okay if I force push a rebase of this branch, or do y'all prefer a fresh PR? |
@daneah This is on my list to review today. A force push is totally fine! |
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.
This looks pretty good! A few thoughts:
- We probably want to unset
yanked_reason
when the release is unyanked. - We should surface
yanked_reason
in the JSON API as well (and add it to the corresponding docs) - I think the
yanked_reason
in the JSON API should probably beNone
when the release is not yanked - We probably want to display the
yanked_reason
in the "Yanked Releases" table as well - We should include
yanked_reason
in theadditional
field when we callrecord_event
during yanking
For tests, I would expect to see updates for yanked_reason
anywhere where we're doing assert release.yanked
or assert not release.yanked
to ensure this field is actually getting set in the view. Same for the JSON API tests.
warehouse/migrations/versions/30a7791fea33_add_yanked_reason_column_to_release_.py
Outdated
Show resolved
Hide resolved
@di thanks for the review! I can definitely address some of those points readily, and I'm sure I'll have a couple of follow up questions 😄 will report back. |
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.
Thanks @daneah! |
Resolves #7856
Fixes #7886