-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add plugin list #7921
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 plugin list #7921
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.
Thanks @mcsitter!
This looks great for starters. Here's the direct link for the preview:
https://pytest--7921.org.readthedocs.build/en/7921/plugin_list.html
I've left a few comments, please take a look. 👍
.github/workflows/plugin-list.yml
Outdated
run: python scripts/plugin_list.py | ||
- name: Create Pull Request | ||
uses: peter-evans/create-pull-request@v3 | ||
with: |
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.
The PR opened by this will target master
, correct?
master
only generates the documentation for https://docs.pytest.org/en/latest, while stable
points to the latest tagged release, and by default users are directed to the stable
version when typing docs.pytest.org
.
Would it be possible to target the latest released branch somehow? Otherwise the updated list of plugins will only be available on latest
, and the default location reached by users will be outdated in general.
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.
As described here:
base | Sets the pull request base branch. | Defaults to the branch checked out in the workflow. |
---|
This actually makes a lot of sense to me. We can also pass a ref to the checkout according to its documentation. It says the ref can be tag (like stable) but i don't know how it would open PR from a detached HEAD. This seems to be a problem to me? Since the output of commands can be used in the workflow one could maybe come up with a way to get the correct branch based on the tag and pass it to the ref
option of the checkout. I am not sure how this would work in regards to keeping the master branch up-to-date. Maybe a matrix of environment variables has to be created?
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 am not sure how this would work in regards to keeping the master branch up-to-date.
If we can make it work by just targeting the latest stable, IMHO that's good enough.
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.
Maybe someone could help me out with this one. I do not know how to get the name of the branch with the latest release. I don't think that checking out a tag would work. Also, i do not have the git repo structure set up to test this properly at mcsitter/pytest-plugin-list. Just sorting by the tags on pytest currently spits out the dev version of version 6.2.0 as the latest tag . Should i just put 6.1.x
in there for the branch? That would mean that every minor version requires an update to the workflow.
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.
(Sorry I missed the notification for this one)
I think it is safe to filter every tag which doesn't match \d+\.\d\.x
, so it will leave out the dev tags or any other tag which doesn't match our version scheme for branches.
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.
Really great work @mcsitter! This would be a good addition.
I have some concerns on the current version:
-
Is it possible to do this in a separate repository, and somehow include it in our docs as an external document?
My main concern is the daily commits. For me as a developer it would be annoying to have our main repo filled with daily commits which update this file.
-
The use of the shields as-is is excessive, and needs a different approach. Loading the page sends 790 HTTP requests to img.shields.io, which, if we add it prominently to our docs, would amount to a ddos attack on them. Already for me a lot of the requests fail with status 429 - too many requests.
-
One requirement for being automatically picked up as a pytest plugin is having a
pytest11
entry point. That may be more accurate than matchingpytest-*
? I am not sure if it is include din the metadata however.
- name: Update Plugin List | ||
run: python scripts/update-plugin-list.py | ||
- name: Create Pull Request | ||
uses: peter-evans/create-pull-request@v3 |
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.
Can we do it ourselves with https://github.com/actions/github-script? I prefer to avoid using external actions, especially in little-noticed automatic PRs. If it's difficult, then we should lock it to a specific commit so it doesn't auto-update.
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.
Here is the API reference which is behind the linked action. The README
shows, that the action is basically a js script which calls the API. I have no idea how js works and what the environment of the script (provided variables etc.) is, but i am giving it a shot. Seems like i am not getting the head
right for this workflow.
While we are at it, this discussion is about the target branch of the pull request and could be solved in the same sweep if someone knows how to script this.
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.
Your workflow looks right to me. It fails because head
is invalid
. I think this is because the branch name update-plugin-list
doesn't exist in your repo. You should run a previous step which creates it with a git push --force
command. Also there needs to be some handling of the case where a PR for the branch is already open -- in this case can just reuse it I suppose.
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 guess this point is still open right?
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.
If the github-script or any other roll-our-own solution is too much work, we can keep using peter-evans/create-pull-request@v3
, but we should at least pin it to a specific commit instead of a tag, so if it's somehow maliciously updated, we won't be affected.
Thank you for your feedback, @bluetech! I hope we can make this a great feature 👍
I understand the concern for the "spam" of commits. I share that concern. Maybe a weekly update would solve that to a satisfactory degree? See the linked discussions in the PR body for the arguments made previously. An external repository has a lot of overhead for what this list aims to achieve. Also the state of the plugin list for specific releases of pytest and thus the documentation would not be available, only the current version, which may not be that useful if say you are still on pytest 5 and wanted to see what plugins are available, but the popular ones already updated the requirement for the latest release. The idea is to have 1 script and 1 workflow to manage the whole thing. Of course this is a pull request and this should be discussed to find the preferred solution.
I think we will tackle this according to the proposals in this discussion. I only encountered this issue once i reloaded the page a couple times within a short amount of time and did not think about it before. Of course we should try to minimize the load we put on their page. If anyone knows how we could also reduce the requests to the pypi API ideas are welcome! Should not be that much of a problem though, since the requests are only done in the workflow. We could even add a sleep, but it would greatly increase the duration of the run.
This information does not seem to be available in the metadata of the JSON API. If that metadata is available somewhere else it could be considered. The list will never be perfect though. For example the |
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 seems like... a lot (complexity, frequent commits/PRs, etc.)
could we just link to this instead? https://pypi.org/search/?c=Framework+%3A%3A+Pytest
Unfortunately the "Framework :: Pytest" has been incorrectly used by many projects to indicate they use pytest, not to identify themselves as plugins, which is unfortunate as completely defeated the purpose of the classifier. But you have a point, what do we get from this that we don't get from a simple link to a search, say https://pypi.org/search/?q=pytest-&o=? From the top of my head:
Of course, we have to balance all the benefits with the downsides, but TBH a simple script and a cron job don't seem to me like it would bring too much of a burden (and we can always revisit this in the future if it does). |
this search seems way better: https://pypi.org/search/?c=Framework+%3A%3A+Pytest&q=pytest |
Also realized we can later update the workflow to also run |
Any updates from the commenters on this? I'm still 👍 on the overall idea, barring some changes that are being discussed. |
Is this going to deprecate plugincompat @nicoddemus ? |
I believe so. |
Yep it will most likely be retired, unless someone wants to maintain it. |
Thanks for confirming! Just curious what benefits would maintaining it have over just using the product of this PR? |
Not much... the current testing that the plugincompat does today is not very useful I think. Having a list of plugins in a single place seems simple and useful enough. |
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 think is an improvement over the status quo so we should go ahead and merge this, just a couple of comments.
- name: Update Plugin List | ||
run: python scripts/update-plugin-list.py | ||
- name: Create Pull Request | ||
uses: peter-evans/create-pull-request@v3 |
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.
If the github-script or any other roll-our-own solution is too much work, we can keep using peter-evans/create-pull-request@v3
, but we should at least pin it to a specific commit instead of a tag, so if it's somehow maliciously updated, we won't be affected.
Howdy folks. Sorry I lost track here, anything TODO here that is preventing this from getting merged? |
My two small comments above still need to be fixed, I can handle them if @mcsitter prefers. |
Help on the workflow file is very welcome! Fixed the reponse code handling. |
Co-authored-by: Ran Benita <[email protected]>
Co-authored-by: Ran Benita <[email protected]>
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.
OK, pushes a couple changes to pin the github action and fix a doc reference, other than that it's good! I'm assuming also from a comment above the @nicoddemus approves as well, so I'll go ahead and merge.
Thanks for this effort @mcsitter!
Thanks for the feedback and helping out! I am happy to contribute. |
Cool! Thanks everyone, specially to @mcsitter for the contribution and patience to get this in! 👍 |
Features
As described in #5105 and mcsitter/pytest-plugin-list#1 this adds a pytest plugin list.
Previously a list of pytest plugins was retrieved from the package index and integration tests were performed. The table showed success and failure of the integration tests of the plugin for different python versions. This did not prove to be very useful so a simpler approach is desired.
A pytest plugin is defined as any package that matches the pattern pytest-*. Packages with the development status set to inactive as indicated by the trove classifier of the package are excluded.
Columns
The plugin list contains the following columns:
Updates
A workflow runs
plugin_list.py
daily and automatically opens a PR for the updates. This PR should simply be merged automatically if the docs tests pass. If the PR is not merged, changes from future runs will be force pushed to the PR instead of opening a new one.Closes #5105