-
Notifications
You must be signed in to change notification settings - Fork 392
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
Reorganize GitHub Actions #1041
Conversation
Well I'm not sure I am using |
Codecov Report
@@ Coverage Diff @@
## main #1041 +/- ##
==========================================
- Coverage 99.02% 98.99% -0.04%
==========================================
Files 119 119
Lines 11030 11030
==========================================
- Hits 10923 10919 -4
- Misses 107 111 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
bfbfb07
to
73e3b0d
Compare
I have removed the trigger on |
👍 I'm traveling this week so won't have much time to look at this till maybe Tuesday. |
Sure this can wait! No hurry at all. And thanks for helping me with this! |
This is up to the settings of the repo. By default someone has to approve the workflow before it runs. The PR author can always make a PR on their own forks to run the jobs 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.
Hope the code review helps. I can't review the codeql part because I'm not familiar with it. But shouldn't it be in the CI?
@@ -3,37 +3,16 @@ on: | |||
push: |
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.
Same trigger as above
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 see. At #1084 I have keep everything in the same file for now.
python -m pip install wheel jupyter-packaging jupyterlab>=3 | ||
BUILD_JUPYTERLAB_EXTENSION=1 python setup.py sdist bdist_wheel | ||
# Don't publish a tar.gz file over 1MB (Issue #730) | ||
if (($(wc -c < dist/*.tar.gz) > 1000000)); then exit 1; fi |
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.
What's the deal with this test? It should be in the CI.
But beware that distro pacakagers rely on the fact that sdist
is a superset of git archive
. Of course you can remove files like .github
, but consult with a distro packager to be safe. E.g. the maintainers of python3-jupytext
on fedora. I can do a quick lookover if you point me to the sdist ignore list.
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 agree that this belongs to the CI.
I am not sure I am following your second comment, sorry. This file: MANIFEST.in controls what goes into the package.
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.
My reading of that check was so that you make sure that large filea are not being packaged right? Well about that, they should be packaged because distro maintainers expect that all relevant files are available on PyPI as well as other generated files.
Thank you @LecrisUT for your comments. Very helpful. It might take me some time to address them all but I'll surely try. |
Closed in favor of #1084 |
Thank you @matthewfeickert for your recommendations at #1037 !
I have tried to implement those in this PR. If you have a minute to have a look that would be great! Thank you.