-
Notifications
You must be signed in to change notification settings - Fork 17
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
Migrating CI from cirrus to GHA #271
Conversation
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## main #271 +/- ##
==========================================
+ Coverage 98.73% 98.79% +0.05%
==========================================
Files 30 33 +3
Lines 3323 3563 +240
==========================================
+ Hits 3281 3520 +239
- Misses 42 43 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Nice job @HGWright! I'm sure @stephenworsley is extra glad you've put in the work, so he has been able to focus on the code-base.
Some modifications for you...
.github/workflows/benchmark.yml
Outdated
uses: actions/checkout@v3 | ||
with: | ||
fetch-depth: 0 | ||
- run: git checkout HEAD^2 |
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.
Worth a comment here, to explain that this checks out the HEAD
of the Pull Request, instead of using GHA's default simulated merge commit.
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 block should also be conditional - only run when on a pull request.
The same action also runs post-merge, and in that situation should run on the default commit with no modifications.
That's why the below line also has a similar check:
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.
see bb271a9
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.
* add nearest neighbour regridding * fix tests * further improvements, reintroduce mask keywords * fix tests * change test name * add tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * flake fix * update changelog * address review comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * flake fix * address review comments * parameterise test * add Raises section to docstrings * fix docstrings * fill out __all__ * address review comments * further test parameterisation. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com>
Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com>
Cracking job @HGWright! And thanks @stephenworsley for helping it over the line. |
All of the CI performed by the cirrus has now been made into separate GHA.
A couple of other files also had to change to facilitate this migration.
closes #265