-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 Tidelift alignment action and badge #5763
Conversation
Wonder why this failed:
https://github.com/python-pillow/Pillow/pull/5763/checks?check_run_id=3897935345 When the first one passed:
https://github.com/python-pillow/Pillow/runs/3897866038?check_suite_focus=true |
If the workflow requires secrets to run, we'll need a guard to prevent it running and failing on forks. For example: jobs:
build:
+ if: github.repository_owner == 'python-pillow'
name: Run Tidelift to ensure approved open source packages are in use |
I've pushed a commit so that there's now a badge at https://pillow--5763.org.readthedocs.build/en/5763/ |
Hi. What's the reason for this being 'Do Not Merge'? Is it just because it's failing, I think due to missing secrets? I also see https://github.com/python-pillow/Pillow/runs/4124628947#step:4:7 ?
|
Yes, that's the main reason: there's no point merging this and then it then fails every build. I also have questions about what the action is actually for: #5762 (comment) |
Right. Let me get back to this now and provide an update by EOM. |
OK added @hugovk suggestion and double checked TIDELIFT_API_KEY, if it still fails I'll check API key again but I think it's set correctly. Thanks @hugovk @radarhere ! |
@JeffStern Is ae4d5d9 correct for org and proj? Thx |
That looks good to me. Are you still seeing issues? |
Thanks! API token looks good now, but fails like this. Should we use https://github.com/tidelift/alignment-action instead?
https://github.com/python-pillow/Pillow/runs/4386180151?check_suite_focus=true |
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@hugovk @radarhere @JeffStern OK here's where it gets potentially "wacky". I hate to add more files to the root of the Pillow repository, but in this case the two Pipenv files I've added may satisfy the Tidelift CI 🤞 (https://support.tidelift.com/hc/en-us/articles/4406286692756-How-to-generate-lockfiles-from-manifests) What's particularly "wacky" for me is I've never, ever, EVER used Pipenv before now. At a glance, it looks like something some people use and others maybe don't use … I certainly don't see Pipfiles in every Python package repository. So, if the CI passes and no one minds the intrusion of two more files in the root, I say we merge for experiment's sake. And if anyone has any relevant Pipenv experience share-away! Thanks all 🙏 |
@JeffStern OK looks like we're just waiting for babel and sphinx to get approved ^^^ thanks! |
pipenv is used for pinning dependencies (and sub-dependencies) to get reproducible builds, usually used for developing applications rather than libraries like Pillow. (We use If possible, it would be nice to avoid pipenv, because it means we need to duplicate the content of Can we have them listing the same things? That is, add packaging and remove docutils from Pipfile? Also, is it possible to give access to the rest of the Pillow team to see the scans, so we can see why they fail? At https://tidelift.com/scans/e4fc2d1a-f253-4831-9ff6-bebe624a75f1 I see:
https://github.com/python-pillow/Pillow/runs/4393978672?check_suite_focus=true |
@hugovk I'll let @JeffStern address adding Pillow team to Tidelift dashboard, but for now let's just assume that failures are caused by babel and sphinx not being in the catalog. I'm not even sure where babel comes from, I ran
I'm tempted to remove all the packages and re-add them one-by-one. |
That command only worked for Black. Doing a reverse search, babel is from Sphinx:
|
env: | ||
TIDELIFT_API_KEY: ${{ secrets.TIDELIFT_API_KEY }} | ||
TIDELIFT_ORGANIZATION: team/aclark4life | ||
TIDELIFT_PROJECT: pillow |
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.
TIDELIFT_PROJECT should be name of exact package repo name right? or anything tidelift specific
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 got it by trial n error. now only figure out tidelift API key authenticate failure
@@ -0,0 +1,22 @@ | |||
[[source]] |
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.
is this file mandatory for tidelift?
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.
@auvipy If you want to provide a lock file, then either Pipfile.lock or poetry.lock according to https://support.tidelift.com/hc/en-us/articles/4406293161492-Compatible-languages-and-package-files
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 I am familiar with both, but not so with tidelift CLI :D I was able to make tidelift authenticate work though
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.
but should I put key in a secret env variable in the local tidelift project?
Almost there! To pass CI linting,
https://support.tidelift.com/hc/en-us/articles/4406293161492-Compatible-languages-and-package-files says that while "Tidelift can generate the most complete bill of materials if we have both files", it seems to suggest that But I'm fine with including it for now just to get it merged, and we can look into removing it later. |
Done, thanks!
It says that, but the UI indicates otherwise, and the issue of (or confusion about) whether or not locking is appropriate in any given context remains (for me at least). I added the lock file to make Tidelift UI warning go away … but yeah, we can decide later if we want to remove it. Particularly if @JeffStern makes it so we optionally don't have to look at that warning in the Tidelift UI (e.g. by adding a With lockfile: 🎉 Without lockfile: 😡 |
my alignments were working without lockfile, or may be false positive? |
@auvipy Right, alignment without lock file is "real". Just complaining about the UI while we're on this thread. 😄 |
Thanks all! 🎄 |
#5762