feature: reference workflowpattern package to add workflow filtering
Description
This is a followup to https://github.com/nektos/act/pull/1618 by @ChristopherHX - which added in a package to accommodate filtering. This PR works to invoke the filtering when the act command is executed.
This PR is in a draft state - there are a number of questions that I'm not in a position to answer (which are outlined in the "Questions" section below), and I can't complete this PR without these questions being answered. Feedback/answers to my questions are much appreciated.
Changes Thusfar
- Add a
FindFilterPatterns()function to theWorkflowstruct.- This function checks to see if the specified event supports filtering (i.e.
pull_requestorpush), it then traverses the nodes inWorkFlow.RawOnand builds up a struct (FilterPatterns) containing slices of filters.
- This function checks to see if the specified event supports filtering (i.e.
- Add a
ShouldFilterWorkflow()function to theWorkflowstruct.- This function calls
FindFilterPatterns()to get the filters patterns (relating to the specified event) on the workflow, it then compiles these patterns into Regex, and compares the provided event against the patterns (to determine whether the workflow should be added to the plan).
- This function calls
- Define a function signature (
workflowpattern.FilterInputsFunc)- this is the signature that both
workflowpattern.Skip()andworkflowpattern.Filter()implement.
- this is the signature that both
Notes
- I had initially looked at changing
RawOnto a struct, so that we could directly unmarshal any filters found in the workflow into this struct.- I decided against this as it would require refactoring/regression testing anything that already makes use of
RawOn.
- I decided against this as it would require refactoring/regression testing anything that already makes use of
- There are a number of
TODOcomments in my code - some of these are the basis for questions below.
Questions
- How should I provide the Github
eventto theShouldFilterWorkflow()function? Since I will probably callShouldFilterWorkflow()within the functions onmodel.workflowPlannerstruct, should I store a copy of theeventon this struct?- The
runner.runnerImpl.configure()function builds up aneventJsonattribute, but none of these functions/structs are exported.- At the time that we plan events/jobs (in
cmd.newRunCommand()), we haven't calledr, err := runner.New(config), so this structure wouldn't be available to theplanner- even if the attribute was exported.
- At the time that we plan events/jobs (in
- The
model.GithubContextstruct appears to relate to the Github event, but there's no function to unmarshal a JSON file into the struct.
- The
- Do we need to check the workflow filters when calling
workflowPlanner.PlanJob()? Filters are a workflow-level attribute (and a Job exists as a component of a workflow file) but some users may wish to invoke a job without the context of its wider workflow. - Should I alter my existing functions (
FindFilterPatterns()andShouldFilterWorkflow()) to return errors instead of callinglog.Fatal()?- I used
log.Fatal()in line with the other functions in themodelpackage, as I saw very fewerrorreturn values on function signatures. - Personally, I prefer returning errors from packages as they're easier to test. I also saw a PR (https://github.com/nektos/act/pull/1705) which is working on replacing
log.Fatal()
- I used
- If users provide events (using the
-eflag) that don't satisfy the conditions on their workflow file, the addition of filtering may break the workflows for those users. Is there a preferred way to communicate this change to those users?
@ae-ou this pull request has failed checks ðŸ›
Codecov Report
Merging #1709 (772062e) into master (4989f44) will increase coverage by
1.27%. The diff coverage is64.21%.
:exclamation: Current head 772062e differs from pull request most recent head 64bef7a. Consider uploading reports for the commit 64bef7a to get more accurate results
@@ Coverage Diff @@
## master #1709 +/- ##
==========================================
+ Coverage 61.22% 62.49% +1.27%
==========================================
Files 46 51 +5
Lines 7141 8282 +1141
==========================================
+ Hits 4372 5176 +804
- Misses 2462 2715 +253
- Partials 307 391 +84
| Impacted Files | Coverage Δ | |
|---|---|---|
| pkg/common/outbound_ip.go | 0.00% <0.00%> (ø) |
|
| pkg/container/docker_cli.go | 82.23% <ø> (ø) |
|
| pkg/container/docker_logger.go | 52.08% <ø> (ø) |
|
| pkg/container/docker_run.go | 13.58% <0.00%> (-0.01%) |
:arrow_down: |
| pkg/container/docker_volume.go | 0.00% <ø> (ø) |
|
| pkg/container/file_collector.go | 39.68% <0.00%> (+2.38%) |
:arrow_up: |
| pkg/container/host_environment.go | 0.00% <0.00%> (ø) |
|
| ...ontainer/linux_container_environment_extensions.go | 23.07% <0.00%> (-1.25%) |
:arrow_down: |
| pkg/exprparser/functions.go | 66.32% <0.00%> (-1.04%) |
:arrow_down: |
| pkg/container/docker_pull.go | 32.58% <22.22%> (-0.75%) |
:arrow_down: |
| ... and 26 more |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
If users provide events (using the -e flag) that don't satisfy the conditions on their workflow file, the addition of filtering may break the workflows for those users. Is there a preferred way to communicate this change to those users?
I have had thought about this a while ago. I would initially apply the path filter only to watch mode.
How should I provide the Github event to the ShouldFilterWorkflow() function?
No idea, maybe the workflowplanner can add a filter function to pass a function to decide which workflow to remove from the workflowplanner object, after logging the exact reason why it won't run. Then you can reference the full payload object to decide if you want to skip it.
Do we need to check the workflow filters when calling workflowPlanner.PlanJob()? Filters are a workflow-level attribute (and a Job exists as a component of a workflow file) but some users may wish to invoke a job without the context of its wider workflow.
No idea, the planner wasn't my idea. It seems act merges all workflows into a single planner object.
@ae-ou this pull request has failed checks ðŸ›
@ae-ou this pull request has failed checks ðŸ›
@ae-ou this pull request is now in conflict 😩