-
Notifications
You must be signed in to change notification settings - Fork 10
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
CI: Add test workflow for action #27
Conversation
.github/workflows/ci.yml
Outdated
|
||
on: | ||
push: | ||
# branches: [ main ] |
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.
Using for debugging. Make sure to revert comment before merge.
1ed424a
to
ff8a2d7
Compare
90d7eaf
to
b25c7f3
Compare
b25c7f3
to
0036838
Compare
Note to reviewers: The CI fails because GitHub Actions is using my fork's credentials to try to run this (as this workflow doesn't exist on the main repo yet) (c.f. https://github.com/scientific-python/upload-nightly-action/pull/27/files#r1270228235). I pushed this branch under a different name as a test to the main repo and it was able to run and upload So this is working as I would expect, and so is ready for your reviews. 👍 |
Gentle ping for review when people have time. |
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.
LGTM!
Maybe we should link this test action from the root README file too?
For example for networkx, we just build and upload. Maybe we should add the verification and debugging steps too 😅
.github/workflows/ci.yml
Outdated
branches: [ main ] | ||
# Run daily at 1:23 UTC | ||
schedule: | ||
- cron: '23 1 * * *' |
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.
Does this really have to run daily? Weekly, or biweekly should be enough for such a repo.
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.
Why not?
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.
Because we don't expect fast pace breakings here, as we practically have very few dependencies, and it's totally unnecessary use of resource.
In fact, as I see all versions are pinned, so then there is zero sense in running the cron.
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.
In fact, as I see all versions are pinned, so then there is zero sense in running the cron.
No, the action isn't fully specified (that requires a lock file, which is an upcoming PR), and the ci
workflow uses unpinned versions of other actions that you want to know about. So testing on CRON is a good idea. The resource use is trivial IMO, but if it is really offensive we can move it to weekly.
uses: ./ | ||
with: | ||
artifacts_path: dist | ||
anaconda_nightly_upload_token: ${{ secrets.UPLOAD_TOKEN }} |
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.
So where would this test package end up being uploaded? Is there any cleanup planned once the upload is successful?
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.
So where would this test package end up being uploaded?
https://anaconda.org/scientific-python-nightly-wheels/test-package
(c.f. #27 (comment))
Is there any cleanup planned once the upload is successful?
No, but does there need to be?
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.
Yes. I would definitely like to avoid polluting the user deployed space with testing artifacts.
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 would definitely like to avoid polluting the user deployed space with testing artifacts.
Can you elaborate as to why this matters? Also please suggest an alternative location.
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'm not suggesting another location, but to do a cleanup. Easy and simple and the pollution will be only present when there is a problem.
@MridulS We could do that, but I would suggest not using this GHA workflow but creating a template that anyone in the org could use, as this workflow is focused on just testing the test package. There are already other projects and Actions that do this and more, like https://github.com/hynek/build-and-inspect-python-package, but I could understand the interest in an action template that would be more specific to a minimum set of verification stages that people can then extend if they want. This would be a separate project though IMO. While I have personally found benefits in |
0036838
to
7919142
Compare
.github/workflows/ci.yml
Outdated
# Run weekly at 1:23 UTC | ||
schedule: | ||
- cron: '23 1 * * 7' |
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.
CRON is now weekly.
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.
Oh this is apparently non-standard CRON syntax? Let me fix 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.
'23 1 * * 7'
is nonstandard because of the 7
so I swapped this to 0
.
.github/workflows/ci.yml
Outdated
cleanup: | ||
runs-on: ubuntu-latest | ||
needs: [test] | ||
# Set required workflow secrets in the environment for additional security | ||
# https://github.com/scientific-python/upload-nightly-action/settings/environments | ||
environment: | ||
name: remove-old-wheels | ||
|
||
steps: | ||
- name: Install micromamba and anaconda-client | ||
uses: mamba-org/setup-micromamba@v1 | ||
with: | ||
environment-name: remove-wheels | ||
create-args: >- | ||
python=3.11 | ||
anaconda-client | ||
|
||
- name: Remove test package upload | ||
run: | | ||
anaconda --token ${{ secrets.ANACONDA_TOKEN }} remove \ | ||
--force \ | ||
"scientific-python-nightly-wheels/test-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.
This should now remove the test-package
package that gets uploaded just previously in the test
job.
7919142
to
b7c5ac0
Compare
* Add simplest example package to tests so that it can be built for upload to Anaconda cloud as a test of the action. - Use setuptools as the build backend over something more obvious like hatchling given the simple nature of the test-package given that anaconda-client v1.12.0 doesn't understand valid metadata from PEP 518 build backends other than setuptools. c.f. anaconda/anaconda-client#676 * Add CI GitHub Action workflow to test the functionality of the action as shown in the README.
b7c5ac0
to
0d8a131
Compare
anaconda-client | ||
|
||
- name: Remove test package upload | ||
shell: bash -l {0} |
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.
Okay final burst of noise (sorry) to add in this shell
line so that conda
works in Actions. This works as expected (just tested on a debug branch on the main project).
👋 @bsipocz if you get time before the end of the week can you review if the changes I made given your original comments address them? |
it would be super nice to allow a quick re-review if commits made after the review were not squashed into the original one. We do squash on merge anyway, so squashing in the PR has no actual benefits. |
@bsipocz see #27 (review) |
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 can't spot any more issues, let's try and see it indeed passes after merge.
Resolves #15
Add simplest example package to tests so that it can be built for upload to Anaconda cloud as a test of the action.
setuptools
as the build backend over something likehatchling
given the simple nature of thetest-package
given thatanaconda-client
v1.12.0
doesn't understand valid metadata from PEP 518 build backends other thansetuptools
. c.f. anaconda-client doesn't know how to read valid metadata from non-setuptools build backends anaconda/anaconda-client#676Add CI GitHub Action workflow to test the functionality of the action as shown in the README.
upload-nightly-action/README.md
Lines 10 to 17 in 4792bc2