act icon indicating copy to clipboard operation
act copied to clipboard

feature: reference workflowpattern package to add workflow filtering

Open ae-ou opened this issue 3 years ago • 7 comments

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 the Workflow struct.
    • This function checks to see if the specified event supports filtering (i.e. pull_request or push), it then traverses the nodes in WorkFlow.RawOn and builds up a struct (FilterPatterns) containing slices of filters.
  • Add a ShouldFilterWorkflow() function to the Workflow struct.
    • 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).
  • Define a function signature (workflowpattern.FilterInputsFunc)
    • this is the signature that both workflowpattern.Skip() and workflowpattern.Filter() implement.

Notes

  • I had initially looked at changing RawOn to 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.
  • There are a number of TODO comments in my code - some of these are the basis for questions below.

Questions

  1. How should I provide the Github event to the ShouldFilterWorkflow() function? Since I will probably call ShouldFilterWorkflow() within the functions on model.workflowPlanner struct, should I store a copy of the event on this struct?
    1. The runner.runnerImpl.configure() function builds up an eventJson attribute, but none of these functions/structs are exported.
      1. At the time that we plan events/jobs (in cmd.newRunCommand()), we haven't called r, err := runner.New(config), so this structure wouldn't be available to the planner - even if the attribute was exported.
    2. The model.GithubContext struct appears to relate to the Github event, but there's no function to unmarshal a JSON file into the struct.
  2. 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.
  3. Should I alter my existing functions (FindFilterPatterns() and ShouldFilterWorkflow()) to return errors instead of calling log.Fatal()?
    1. I used log.Fatal() in line with the other functions in the model package, as I saw very few error return values on function signatures.
    2. 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()
  4. 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?

ae-ou avatar Apr 02 '23 22:04 ae-ou

@ae-ou this pull request has failed checks 🛠

mergify[bot] avatar Apr 08 '23 14:04 mergify[bot]

Codecov Report

Merging #1709 (772062e) into master (4989f44) will increase coverage by 1.27%. The diff coverage is 64.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

codecov[bot] avatar Apr 08 '23 15:04 codecov[bot]

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.

ChristopherHX avatar Apr 13 '23 12:04 ChristopherHX

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.

ChristopherHX avatar Apr 13 '23 12:04 ChristopherHX

@ae-ou this pull request has failed checks 🛠

mergify[bot] avatar Jun 19 '23 22:06 mergify[bot]

@ae-ou this pull request has failed checks 🛠

mergify[bot] avatar Jul 18 '23 08:07 mergify[bot]

@ae-ou this pull request is now in conflict 😩

mergify[bot] avatar Nov 12 '23 20:11 mergify[bot]