-
Notifications
You must be signed in to change notification settings - Fork 4
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
Move startGroup/endGroup constructs to individual fields #35
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.
LGTM, thanks!
Up to you when to merge - I already effectively did this by overriding |
BTW I've added some extra documentation on startGroup/endGroup (to document that they not only create foldable groups using the standard GHA feature, but also compute the intermediate elapsed time). I'd like to merge this PR ASAP as it is a fix, however I'd be glad to have some review for the doc I have added (in a2a4cb7) Cc @Zimmi48 FYI |
In the meantime, note that even if this PR is not yet merged, the docker-coq-action can already be used as is by specifying the temporary branch - uses: coq-community/docker-coq-action@fix-groups |
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 but please fix the example overriding before_script
, script
and uninstall
to create groups (in particular, this would replace the echo Workaround...
).
Impact: * this slightly changes defaults values w.r.t. v1.2.0 * but this is fully backward-compatible w.r.t. v1.1.0 * and this makes it possible to customize group names (that cannot be nested)
in order to be more robust and future-proof, if users happen to write more involved group titles, involving special characters to be quoted.
Fixed, and will now release v1.2.1. Thanks @Zimmi48 for your review! |
Release done: https://github.com/coq-community/docker-coq-action/releases/tag/v1.2.1; so you'll be able to use the default |
Thanks @erikmd, I'm now taking advantage of this in Perennial. |
Impact:
I can readily do a point release if you want @tchajed ; otherwise tomorrow Saturday :)