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

Implement PEP 592: Yanked Releases #5838

Merged
merged 34 commits into from
Apr 22, 2020
Merged

Implement PEP 592: Yanked Releases #5838

merged 34 commits into from
Apr 22, 2020

Conversation

dstufft
Copy link
Member

@dstufft dstufft commented May 12, 2019

  • Update the Release model to support marking a release as yanked.
  • Update the simple API so that it exposes the data-yanked attribute.
  • Provide an API for yanking/unyanking a release.
  • Tests

@nlhkabu
Copy link
Contributor

nlhkabu commented Sep 15, 2019

Note:
If / when this PR is re-reviewed, we will need to ensure that the templates are appropriately translated.

@brainwane
Copy link
Contributor

This feature would be helpful in improving pip's test automation so it would be great if you could prioritize finishing this WIP.

@dstufft
Copy link
Member Author

dstufft commented Jan 27, 2020

I am unlikely to find time in the interim to finish this, though I think we could deploy this without an API and just have it be a UI only feature for now.

@di di changed the title [WIP] Implement PEP 592: Yanked Releases Implement PEP 592: Yanked Releases Mar 13, 2020
@di di requested a review from ewdurbin March 13, 2020 21:15
@brainwane
Copy link
Contributor

@ewdurbin @di @dstufft hey - as Ernest and @pradyunsg and I talked about in a meeting about the pip resolver work, getting this implemented might help us smooth the second-order effects of the resolver rollout, so if you could help get this finished and merged, we'd appreciate that!

@di
Copy link
Member

di commented Apr 2, 2020

@nlhkabu, could you take a look at 1d7824d if you have a second? Without this change, the callout blocks were appearing above the modals.

@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 3, 2020

@di 1d7824d looks fine to me.

I have updated the copy in a couple of places (fixed formatting, typo), and changed the icon:

Screenshot from 2020-04-03 19-55-13

Hope this is ok @dstufft

@ewdurbin
Copy link
Member

ewdurbin commented Apr 3, 2020

The value of yanked be added to the JSON documents for projects/releases, rather than hiding them.

Copy link
Member Author

@dstufft dstufft left a comment

Choose a reason for hiding this comment

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

I think my biggest thing is I'm not sure that we want yanked to function as hiding releases from the UI (but maybe we do, I'm not sure?).

My gut instinct tells me we probably want this behavior:

  • On the unversioned URL for a project, we won't pick a yanked version, and would instead act as if there were no versions.
    • One could argue that it's better to just display a yanked version as a last ditch effort to display something, I'm not sure though?
  • On a versioned URL, yanked releases will show up normally, except there is some marker on the page to indict this release has been yanked (likely some sort of red warning banner?).
  • On the list of all versions, the yanked releases still show up, but are marked in some fashion to show that they have been yanked (greyed out to demphasize? red? idk).

Fundamentally, I don't think we want to reimplement the hidden release option from legacy PyPI, this is largely about making it so that people can prevent a version from being installed unless absolutely needed, it isn't about hiding it like it never existed and I think that being able to introspect it in the UI still is still useful.

(Release.project == self)
& (Release.canonical_version == canonical_version)
Release.project == self,
Release.yanked.is_(False),
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to do this? This will make it so yanked versions can't be viewed online at all right?

@di
Copy link
Member

di commented Apr 3, 2020

I see where you're coming from @dstufft. My thought is that if we want people to use yanks instead of deleting releases, we need to make yanks behave just like deleting releases, except you can still pin to them. People delete releases for a lot of reasons: bad long_description, typos, etc. Ideally in those cases, maintainers can still "remove" a release from the public eye, but anyone already using it can continue to pin to it.

Another issue this helps avoid is requiring average users to understand what a yanked release even is. If a user is looking at project page for some-project and they see that the latest release, 1.0.0 is marked as "yanked" with a big red box, but it's still there, they're probably not going to understand what's happening when they pip install some-project and get version 0.9.9 instead of 1.0.0. If the project page just shows the un-yanked releases though, nothing changes here, and they never need to know that 1.0.0 even exists.

@dstufft
Copy link
Member Author

dstufft commented Apr 3, 2020

See I almost consider it the other way, people are still using versions of pip that don't support yanked, so they're likely to be very confused if they pip install some-project with a version that supports it, and then a version that doesn't support it and get different results. I presume most of them are going to go to PyPI at that point and then will start looking around trying to figure out why they're getting different behavior, and keeping the versions all in the UI, but appropriately marking the yanked ones as yanked will help them debug what is happening.

I also don't think that the use case for yanked needs to behave "just as if it was deleted". I do think that for the unversioned URL we should certainly not select a yanked version (possibly unless there ONLY exist yanked versions?), so someone would have to explictly seek out a yanked version in order to even see one exists.

If we do that, we might possibly want to mark a yanked release's mage as noindex as well, so that it won't show up in google's search either.

@di
Copy link
Member

di commented Apr 3, 2020

Hmm, I see your point, and I think you're probably right. I hadn't considered the difference in behaviors between clients that do/don't support yanked releases.

@di di force-pushed the yanked branch 2 times, most recently from 468f70a to 9fa75ad Compare April 4, 2020 19:28
@di
Copy link
Member

di commented Apr 4, 2020

@dstufft I think I addressed all your feedback here. Yanked releases will show up everywhere except:

  • Project.latest_version will not return a yanked release as the latest
  • The project detail page will not show a yanked release as the latest
  • /pypi/<project_name>/json will not use a yanked release as the latest
  • Search reindexing will not include yanked releases

warehouse/templates/legacy/api/simple/detail.html Outdated Show resolved Hide resolved
warehouse/templates/pages/help.html Outdated Show resolved Hide resolved
Copy link
Contributor

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I feel like it'd be nice to require users to put in something as "reason for yanking" (picking from a bunch of fixed choices, for example) since installers can present that information to the user (pip prints a warning containing the message), and having it be compulsory from the start would help avoid a potential transition step if we add it later.

Then again, it's much easier to do a smoother transition on web application than a command line application, so it's perfectly fine if y'all think it's OK to not start with that... :)

@di di force-pushed the yanked branch 2 times, most recently from ff61e7a to 63334ac Compare April 6, 2020 14:36
Copy link
Member Author

@dstufft dstufft left a comment

Choose a reason for hiding this comment

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

I can't approve this because it's my own PR, but we'll just pretend I did.

This release will still be installable for users pinning to this exact version, e.g. when using <code>{{ project_name }}=={{ version }}</code>.
{% endtrans %}
{% trans href="https://www.python.org/dev/peps/pep-0592/" %}
For more information, see <a href="{{ href }}">PEP 592</a>.
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't need to block this PR, but I've wondered if our propensity for linking to PEPs is the best user experience we can provide here, or if we should figure out a real documentation pipeline for Warehouse (not really counting what exists now since it's primarily developer facing documentation, not user facing) and start moving to that.

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 I agree, we are sort of hitting the limits of what we can document inline and in /help.

Copy link
Contributor

Choose a reason for hiding this comment

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

I filed #7817 - thanks for the idea, @dstufft

@di di merged commit 69ce3dd into pypi:master Apr 22, 2020
@pradyunsg
Copy link
Contributor

YAY! Thanks for all of your work on this @nlhkabu @di @dstufft @ewdurbin! ^>^

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.

7 participants