-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: GitHub Action customizations #382
base: master
Are you sure you want to change the base?
Conversation
thoughts @jakemac53 @natebosch ? |
'$package; ${task.command}', | ||
_commandForOs(task.command), | ||
id: task.action?.id, | ||
ifCondition: task.action?.condition ?? |
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 think we should likely add the && steps.$pubStepId.conclusion == 'success'
no matter what?
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.
Sounds good to me. I agree that in most cases, this is the desired behavior, and for the few "failure-only" scripts you may want to run, they can be handled via the on_completion
task.
workingDirectory: package, | ||
'$package; ${task.command}', | ||
_commandForOs(task.command), | ||
id: task.action?.id, |
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 am a bit concerned with hard coding all the supported fields we allow overriding here.
Would it be reasonable to just treat this as a more opaque override map? I don't think we want to be fielding feature requests/prs for every thing people end up wanting to override here.
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.
That's a good question. At that point, does it make sense for action
to function more similar to command
and spread its JSON into the GitHub action yaml so that, for example, overriding run
becomes possible? Right now, the following is unclear in my mind:
stages:
- custom_step:
- command: ./script_a
action:
run: ./script_b
But this could possibly make more sense:
stages:
- custom_step:
- action:
run: ./script_b
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.
Yeah, possibly just allowing a fully custom action task would make the most sense? I kind of like that.
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.
Sweet! I’ll work on updating it 👍
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, so I've made the following changes:
action
now functions similar tocommand
as a top level step type- Consolidated GitHub action config into the
GitHubActionConfig
class since there was a lot of overlap with_CommandEntry
- Added some validators for known GitHub step config keys (
run
,uses
,with
, etc.) but allows for unknown keys to be used as well. Let me know if these helpers are useful or if we should just dump whatever we're given. There is the possibility for things to change, and the flipside is that I think it offers a bit nicer DX than discovering a typo down the road.
Hey @jakemac53 - still feeling like this is a good direction? I've updated the |
cc @natebosch or @kevmoo opinions here? |
We want to land this? @jakemac53 ? |
@dnys1 – you'll need to rebase this work on latest. Sorry! |
2fc2ce6
to
f200385
Compare
Hey @kevmoo, no worries and thanks for the heads up. I've rebased off the latest changes. |
dc5436d
to
bc155cb
Compare
Pull Request Test Coverage Report for Build 2693092847
💛 - Coveralls |
Pull Request Test Coverage Report for Build 2693043484
💛 - Coveralls |
Codecov Report
@@ Coverage Diff @@
## master #382 +/- ##
==========================================
- Coverage 84.10% 7.13% -76.97%
==========================================
Files 34 34
Lines 1453 1555 +102
==========================================
- Hits 1222 111 -1111
- Misses 231 1444 +1213
Continue to review full report at Codecov.
|
bc155cb
to
db9505a
Compare
Two issues fixed: 1. `name` overrides were not being passed correctly to methods/ctors - whoops! 2. Default `name` should consider either `run` or `uses` depending on which is passed.
Hi, any plans to continue this? I would love to have this feature implemented. |
Adds support for an
action
block which can be attached to tasks to specify GitHub Action context, e.g.uses
,with
, etc. Instead of a separate block, this config could be in-lined but I wasn't sure if that was in keeping with the library, since providers other than GitHub are supported.These are just some thoughts - looking forward to your feedback!