feature: reference workflowpattern package to add workflow filtering

[ae-ou]

Description

This is a followup to #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 (Avoid using log.Fatal in pkg/* #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?

[mergify]

@ae-ou this pull request has failed checks 🛠

[codecov]

Codecov Report

Merging #1709 (772062e) into master (4989f44) will increase coverage by 1.27%.
The diff coverage is 64.21%.

❗ 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%)	⬇️
pkg/container/docker_volume.go	0.00% <ø> (ø)
pkg/container/file_collector.go	39.68% <0.00%> (+2.38%)	⬆️
pkg/container/host_environment.go	0.00% <0.00%> (ø)
...ontainer/linux_container_environment_extensions.go	23.07% <0.00%> (-1.25%)	⬇️
pkg/exprparser/functions.go	66.32% <0.00%> (-1.04%)	⬇️
pkg/container/docker_pull.go	32.58% <22.22%> (-0.75%)	⬇️
... and 26 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ChristopherHX]

We probably want a logrus tracewriter for consistent logging

it shoud respect --json

[ae-ou]

Hello. I'm looking back at this PR now.

It doesn't look like there's any implementation of your workflowpattern.TraceWriter interface in the Logrus package - their Info function uses the Info(args ...interface{}) stub.

The workflowpattern.TraceWriter Interface doesn't seem to be used anywhere else in nektos/act - should the interface be altered to comply with the Logrus.Info(args ...interface{}) function stub, or did you initially plan to create an implementation of workflowpattern.TraceWriter that wraps around Logrus?

[ChristopherHX]

I wrote workflow pattern in a way to avoid any non golang stdlib.

For example if we want to change the logging system, this package doesn't need to change.

fmt.Sprintf can create a string you can pass to logrus similar to what the default impl. does.

I expect the caller of the package to create a small adapter struct.

I can create a logrus adapter for you, it's not time expensive.

Maybe just add an interface adapter for logrus like LogrusLogger(some custom interface with the logging signatures of logrus) returns the existing interface without logrus import.

[ChristopherHX]

Actually we can just append a f to Info in this package and it is an exact match of a logrus logger instance

Logrus.Infof should not be used, use the logger instance of act. Not a static wrapper of the standard logger

[ae-ou]

I've updated the TraceWriter interface so that Info is now Infof.

Logrus.Infof should not be used, use the logger instance of act. Not a static wrapper of the standard logger

Apologies if I'm being a bit dense, but I'm not sure that I'm following you. Do you want me to do dependency inversion here? As in - specify the TraceWriter interface as a parameter for my functions/as an attribute on the Workflow struct (which my functions receive), so that I can then pass in a concrete Logrus instance (since it now lines up with the interface) from upstream?

[ChristopherHX]

I mean pass common.Logger(ctx) to the workflow pattern package as Tracewriter if possible (it returns a logrus logger object).

The logrus package has Infof Both as an interface Method and as an static package function, but the logrus interface has name FieldLogger/Ext1Logger not logrus.
Sorry for the confusion.

You can keep logrus interfaces in all other packages, because they are already using logrus directly.

[ChristopherHX]

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]

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.

[mergify]

@ae-ou this pull request has failed checks 🛠

[mergify]

@ae-ou this pull request has failed checks 🛠

[mergify]

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