-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Amend upstream-dev GitHub action + Bug fixes #4604
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
Conversation
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.
looks good to me. I don't know enough about github actions to tell whether this actually works, though.
We might be able to avoid the extra step, but this comes at the cost of a slightly higher complexity (see below).
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 looked over the workflow file and noticed a few more things I would change.
Also, we should probably refactor the parse script, I think being able to pass the files via command line helps a lot when testing the script locally. I added a modified version to my test project which parses the short test summary info lines (but feel free to edit it if you think something is hard to read or if it is too fragile). The call in the CI file would then have to be
shopt -s globstar
python .github/workflows/parse_logs.py logs/**/*-log
Edit: actually, let's make that a new PR
When you get a chance, this is ready for another round of 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.
thanks, @andersy005, looks good to me.
One more point - now the Actions also run on our personal forks. Is that something we want or should/ can this be disabled? |
I don't think we should do that, running the same CI on the same code multiple times in parallel doesn't make much sense. We might be able to stop that by tweaking the |
alright then, let's merge on green. This is also for a different PR, but maybe we can add a "import xarray" step. This is useful because in case some required dependencies are missing (like |
Thanks @andersy005 & @keewis ! |
whats-new.rst