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

Check plugin version before indexing #624

Merged
merged 2 commits into from
Aug 4, 2022
Merged

Conversation

liu-ziyang
Copy link
Contributor

@liu-ziyang liu-ziyang commented Aug 3, 2022

this address some problems identified in #611

perform a version check when indexing plugin metadata to prevent pypi api race condition, where the listing reports a newer version but history API is not up to date to use that version.

lgan-czi
lgan-czi previously approved these changes Aug 3, 2022
Copy link
Contributor

@lgan-czi lgan-czi left a comment

Choose a reason for hiding this comment

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

LGTM

@lgan-czi lgan-czi self-requested a review August 3, 2022 18:19
@lgan-czi lgan-czi dismissed their stale review August 3, 2022 18:20

Wait hang on, I remember we used get_plugin_pypi_metadata somewhere else too, would that get affected?

@lgan-czi
Copy link
Contributor

lgan-czi commented Aug 3, 2022

The other place it's used is in is preview.py, not sure if preview.py is affected by the non-versioned pypi API not updating correctly
Screen Shot 2022-08-03 at 11 23 53 AM

Copy link
Contributor

@klai95 klai95 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@richaagarwal richaagarwal left a comment

Choose a reason for hiding this comment

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

(GitHub won't let me delete this stale review - see latest comment)

Copy link
Collaborator

@richaagarwal richaagarwal left a comment

Choose a reason for hiding this comment

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

@potating-potato Thanks for looking into this! Some questions/thoughts:

Is the idea that we would wait for PyPI's non-versioned endpoint to "catch up" to the latest release? #611 was originally opened due to an observed lag (>4 hours) before certain plugins were showing up at all on the hub (see #611 (comment)). Do you think this lag is due to this issue with PyPI's non-versioned endpoint being out of date? If so, we would still expect this lag to continue, right?

I'm okay with merging this to fix the particular versioning conflict issue you summarized in #611 (comment), but let's leave #611 open so we can also address the lag -- it seems like PyPI's non-versioned endpoint isn't as reliable as we might've thought, and we might need to consider implementing option 2 in #608 sooner than later.

@liu-ziyang
Copy link
Contributor Author

@potating-potato Thanks for looking into this! Some questions/thoughts:

Is the idea that we would wait for PyPI's non-versioned endpoint to "catch up" to the latest release?

Yes

we would still expect this lag to continue, right?

I am assuming so at the moment, I will keep 611 open

@liu-ziyang liu-ziyang merged commit b43ad09 into main Aug 4, 2022
@liu-ziyang liu-ziyang deleted the fix-plugin-versioning branch August 4, 2022 18:00
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.

4 participants