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

Add Tidelift alignment action and badge #5763

Merged
merged 16 commits into from
Dec 25, 2021
Merged

Add Tidelift alignment action and badge #5763

merged 16 commits into from
Dec 25, 2021

Conversation

aclark4life
Copy link
Member

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Oct 14, 2021

Wonder why this failed:

Running alignment. Uploading files. (This may take a few minutes to complete)
Error: invalid authentication. are you using the correct API key? (ptm.tl/docs-apikeys).

https://github.com/python-pillow/Pillow/pull/5763/checks?check_run_id=3897935345

When the first one passed:

Running alignment. Uploading files. (This may take a few minutes to complete)

Revision: 5c69dc7c9a1b12eae14e6ab4823f73da6a5911e7 (Check alignment status by running 'tidelift status 5c69dc7c9a1b12eae14e6ab4823f73da6a5911e7')
+ echo 'Waiting for the alignment to start'
Waiting for the alignment to start
+ sleep 15
+ ./tidelift status --wait 5c69dc7c9a1b12eae14e6ab4823f73da6a5911e7
Waiting for alignment to finish

Alignment Details URL: tidelift.com/scans/842c3013-bd63-4667-8582-8b583c87f43e
Revision: 5c69dc7c9a1b12eae14e6ab4823f73da6a5911e7 - Status: success

https://github.com/python-pillow/Pillow/runs/3897866038?check_suite_focus=true

@hugovk
Copy link
Member

hugovk commented Oct 14, 2021

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

@hugovk hugovk mentioned this pull request Oct 14, 2021
@hugovk hugovk changed the title Add tidelift alignment badge Add Tidelift alignment action and badge Oct 14, 2021
@radarhere
Copy link
Member

I've pushed a commit so that there's now a badge at https://pillow--5763.org.readthedocs.build/en/5763/

@hugovk hugovk marked this pull request as draft October 19, 2021 06:40
@radarhere
Copy link
Member

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 ?

⚠️ Warning: this Github Action is deprecated. Please use https://github.com/tidelift/alignment-action instead.

@hugovk
Copy link
Member

hugovk commented Nov 21, 2021

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)

@aclark4life
Copy link
Member Author

Right. Let me get back to this now and provide an update by EOM.

@aclark4life aclark4life marked this pull request as ready for review December 1, 2021 19:02
@aclark4life
Copy link
Member Author

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 !

@aclark4life
Copy link
Member Author

@JeffStern Is ae4d5d9 correct for org and proj? Thx

@jeffstade
Copy link

That looks good to me. Are you still seeing issues?

@hugovk
Copy link
Member

hugovk commented Dec 1, 2021

Thanks!

API token looks good now, but fails like this. Should we use https://github.com/tidelift/alignment-action instead?

⚠️ Warning: this Github Action is deprecated. Please use tidelift/alignment-action instead.
Downloading latest CLI
+ chmod +x ./tidelift
+ echo 'Current Tidelift CLI version'
Current Tidelift CLI version
+ ./tidelift version
+ echo 'Uploading manifests for alignment'
+ ./tidelift alignment save --revision f26e370ebbb874b3afb1aa6206c5404bf0f08a70 --branch merge --directory /github/workspace
1.2.5-build-4330
Uploading manifests for alignment
Running alignment. Uploading files. (This may take a few minutes to complete)

Revision: f26e370ebbb874b3afb1aa6206c5404bf0f08a70 (Check alignment status by running 'tidelift status f26e370ebbb874b3afb1aa6206c5404bf0f08a70')
+ echo 'Waiting for the alignment to start'
+ sleep 15
Waiting for the alignment to start
+ ./tidelift status --wait f26e370ebbb874b3afb1aa6206c5404bf0f08a70
Waiting for alignment to finish
Error: blocking issues.

Alignment Details URL: tidelift.com/scans/12340224-5aa7-40ef-b05a-9878e540039e
Revision: f26e370ebbb874b3afb1aa6206c5404bf0f08a70 - Status: failure


Blocking issues found. See Alignment Details URL for more detail.

https://github.com/python-pillow/Pillow/runs/4386180151?check_suite_focus=true

aclark4life and others added 2 commits December 1, 2021 17:05
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@aclark4life
Copy link
Member Author

aclark4life commented Dec 2, 2021

@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 🙏

@aclark4life
Copy link
Member Author

Progress …

Screen Shot 2021-12-01 at 7 33 43 PM

@aclark4life
Copy link
Member Author

More progress …

Screen Shot 2021-12-01 at 7 48 39 PM

@aclark4life
Copy link
Member Author

@JeffStern OK looks like we're just waiting for babel and sphinx to get approved ^^^ thanks!

@hugovk
Copy link
Member

hugovk commented Dec 2, 2021

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 requirements.txt for test/docs/release so there is an argument to be made for pinning (and a counter-argument for having to manage/update those pins).)

If possible, it would be nice to avoid pipenv, because it means we need to duplicate the content of requirements.txt into Pipfile and then run a command to regenerate Pipfile.lock, but if it's the only way, so be it. (In general, if another library had runtime dependencies they probably should not pin.)

Can we have them listing the same things? That is, add packaging and remove docutils from Pipfile?

image


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:

Unable to find scan e4fc2d1a-f253-4831-9ff6-bebe624a75f1

Revision: 3d0c10ca6463e7ef35ae4a4a9a8b51f4989df519 (Check alignment status by running 'tidelift status 3d0c10ca6463e7ef35ae4a4a9a8b51f4989df519')
+ echo 'Waiting for the alignment to start'
+ sleep 15
Waiting for the alignment to start
+ ./tidelift status --wait 3d0c10ca6463e7ef35ae4a4a9a8b51f4989df519
Waiting for alignment to finish
Error: blocking issues.

Alignment Details URL: tidelift.com/scans/e4fc2d1a-f253-4831-9ff6-bebe624a75f1
Revision: 3d0c10ca6463e7ef35ae4a4a9a8b51f4989df519 - Status: failure

https://github.com/python-pillow/Pillow/runs/4393978672?check_suite_focus=true

@aclark4life
Copy link
Member Author

@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 pipdeptree on requirements.txt and I don't see it:


╰─[:)] % for i in `cat requirements.txt | grep -v ^# `
do
echo -- $i --
pipdeptree -p $i
done
-- black --
black==19.3b0
  - appdirs [required: Any, installed: 1.4.4]
  - attrs [required: >=18.1.0, installed: 21.2.0]
  - click [required: >=6.5, installed: 8.0.3]
  - toml [required: >=0.9.4, installed: 0.10.2]
-- check-manifest --

-- coverage --

-- defusedxml --

-- docutils==0.16 --

-- markdown2 --

-- olefile --

-- packaging --

-- pyroma --

-- pytest --

-- pytest-cov --

-- pytest-timeout --

-- sphinx>=2.4 --

-- sphinx-copybutton --

-- sphinx-issues --

-- sphinx-removed-in --

-- sphinx-rtd-theme --

-- sphinxext-opengraph --

I'm tempted to remove all the packages and re-add them one-by-one.

@hugovk
Copy link
Member

hugovk commented Dec 3, 2021

That command only worked for Black.

Doing a reverse search, babel is from Sphinx:

$ pipdeptree -p babel --reverse
babel==2.9.1
  - Sphinx==4.3.1 [requires: babel>=1.3]
    - sphinx-copybutton==0.4.0 [requires: sphinx>=1.8]
    - sphinx-issues==1.2.0 [requires: sphinx]
    - sphinx-removed-in==0.2.1 [requires: Sphinx]
    - sphinx-rtd-theme==1.0.0 [requires: sphinx>=1.6]
    - sphinxext-opengraph==0.5.0 [requires: sphinx>=2.0]

env:
TIDELIFT_API_KEY: ${{ secrets.TIDELIFT_API_KEY }}
TIDELIFT_ORGANIZATION: team/aclark4life
TIDELIFT_PROJECT: pillow
Copy link

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

Copy link

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]]
Copy link

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?

Copy link
Member Author

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

Copy link

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

Copy link

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?

@auvipy
Copy link

auvipy commented Dec 12, 2021

Progress …

Screen Shot 2021-12-01 at 7 33 43 PM

I can see security advised PyPI catalog, did you created any custom catalog? is it possible to share that sample to the slack chat?

@aclark4life
Copy link
Member Author

Success (finally)!

Screen Shot 2021-12-20 at 2 29 27 PM

@hugovk
Copy link
Member

hugovk commented Dec 21, 2021

Almost there! To pass CI linting, MANIFEST.in needs updating to include the new files:

lint run-test: commands[1] | check-manifest
lists of files in version control and sdist do not match!
missing from sdist:
  Pipfile
  Pipfile.lock
suggested MANIFEST.in rules:
  include *.lock
  include Pipfile

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 requirements.txt is fine on its own without a lockfile.

But I'm fine with including it for now just to get it merged, and we can look into removing it later.

@aclark4life
Copy link
Member Author

Almost there! To pass CI linting, MANIFEST.in needs updating to include the new files:

lint run-test: commands[1] | check-manifest
lists of files in version control and sdist do not match!
missing from sdist:
  Pipfile
  Pipfile.lock
suggested MANIFEST.in rules:
  include *.lock
  include Pipfile

Done, thanks!

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 requirements.txt is fine on its own without a lockfile.

But I'm fine with including it for now just to get it merged, and we can look into removing it later.

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 [✔️ ] No lockfiles in this project option.) 😄

With lockfile: 🎉

Screen Shot 2021-12-21 at 11 04 02 AM

Without lockfile: 😡

Screen Shot 2021-12-21 at 11 08 41 AM

@auvipy
Copy link

auvipy commented Dec 21, 2021

my alignments were working without lockfile, or may be false positive?

@aclark4life
Copy link
Member Author

@auvipy Right, alignment without lock file is "real". Just complaining about the UI while we're on this thread. 😄

@aclark4life aclark4life merged commit 9d0703a into main Dec 25, 2021
@aclark4life aclark4life deleted the new-badge branch December 25, 2021 12:21
@aclark4life
Copy link
Member Author

Thanks all! 🎄

aclark4life added a commit that referenced this pull request Nov 14, 2022
Not sure if we still care about this? cf. #5762 #5763
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.

5 participants