Skip to content
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.

Sign up for GitHub

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

feature: reference workflowpattern package to add workflow filtering #1709

Closed
wants to merge 7 commits into from
Closed

feature: reference workflowpattern package to add workflow filtering #1709

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9d94a76
Get filter patterns nested within the the On attribute of the workflow.
ae-ou Mar 27, 2023
2a582fd
Add a ShouldFilterWorkflow() function. Testing is still needed for th…
ae-ou Mar 27, 2023
5f88d0a
Switch to FilterStruct
ae-ou Mar 29, 2023
e7702b9
Handle the '*-ignore' filter paths, and return a log.Fatal() if a fil…
ae-ou Apr 2, 2023
9d7a369
Merge branch 'master' into feature-reference-workflow-pattern-package
ae-ou Apr 2, 2023
772062e
Merge branch 'nektos:master' into feature-reference-workflow-pattern-…
ae-ou Jun 11, 2023
64bef7a
Merge branch 'nektos:master' into feature-reference-workflow-pattern-…
ae-ou Jul 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkg/model/planner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ func (wp *workflowPlanner) PlanEvent(eventName string) (*Plan, error) {
continue
}

//TODO: filter out workflow using ShouldFilterWorkflow() - we will need to provide the GitHub/Event context

for _, e := range events {
if e == eventName {
stages, err := createStages(w, w.GetJobIDs()...)
Expand All @@ -208,6 +210,9 @@ func (wp *workflowPlanner) PlanJob(jobName string) (*Plan, error) {
var lastErr error

for _, w := range wp.workflows {
//TODO: do we filter out workflows here? We want to run a specific job, not the full workflow - so should
// the workflow-level filters (e.g. "tags", "branches-ignore") still apply?

stages, err := createStages(w, jobName)
if err != nil {
log.Warn(err)
Expand All @@ -229,6 +234,8 @@ func (wp *workflowPlanner) PlanAll() (*Plan, error) {
var lastErr error

for _, w := range wp.workflows {
//TODO: filter out workflow using ShouldFilterWorkflow() - we will need to provide the GitHub/Event context

stages, err := createStages(w, w.GetJobIDs()...)
if err != nil {
log.Warn(err)
Expand Down
139 changes: 139 additions & 0 deletions pkg/model/workflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package model

import (
"fmt"
"github.com/nektos/act/pkg/workflowpattern"
"io"
"reflect"
"regexp"
Expand All @@ -23,6 +24,17 @@ type Workflow struct {
Defaults Defaults `yaml:"defaults"`
}

// FilterPatterns is a structure that contains filter patterns that were parsed from the On attribute in an event
// https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions
type FilterPatterns struct {
Branches []string
BranchesIgnore []string
Paths []string
PathsIgnore []string
Tags []string
TagsIgnore []string
}

// On events for the workflow
func (w *Workflow) On() []string {
switch w.RawOn.Kind {
Expand Down Expand Up @@ -55,6 +67,133 @@ func (w *Workflow) On() []string {
return nil
}

//FindFilterPatterns searches for filter patterns relating to the specified eventName (e.g. "pull_request", or "push")
//in the RawOn attribute. This will error out if there are filters that can't be used simultaneously for the same event
//(e.g. "paths" and "paths-ignore")
func (w *Workflow) FindFilterPatterns(eventName string) *FilterPatterns {
//Return immediately if the event type doesn't support filters
if eventName != "push" && eventName != "pull_request" {
return nil
}

//If it isn't a mapping node, then the following traversal can't be performed
if w.RawOn.Kind != yaml.MappingNode {
return nil
}

//Decode rawOn to a map of string=>interfaces
var topLevelMap map[string]interface{}
err := w.RawOn.Decode(&topLevelMap)
if err != nil {
log.Fatal(err)
ae-ou marked this conversation as resolved.
Show resolved Hide resolved
}

output := &FilterPatterns{}

//topLevelMapKey correlates to the event type - e.g. "push" or "pull_request"
for topLevelMapKey, topLevelMapVal := range topLevelMap {

//Skip to the next iteration if this isn't the event that we're looking for.
if topLevelMapKey != eventName {
continue
}

if midLevelMap, ok := topLevelMapVal.(map[string]interface{}); ok {
//midLevelMapKey correlates to the filter type - e.g. "branches", "tags", or paths
for midLevelMapKey, midLevelMapVal := range midLevelMap {
if lowLevelMapVal, ok := midLevelMapVal.([]interface{}); ok {
//Leaf correlates to the actual filter pattern
for _, leaf := range lowLevelMapVal {
if leafString := leaf.(string); ok {
switch midLevelMapKey {
case "branches":
output.Branches = append(output.Branches, leafString)
case "branches-ignore":
output.BranchesIgnore = append(output.BranchesIgnore, leafString)
case "paths":
output.Paths = append(output.Paths, leafString)
case "paths-ignore":
output.PathsIgnore = append(output.PathsIgnore, leafString)
case "tags":
output.Tags = append(output.Tags, leafString)
case "tags-ignore":
output.TagsIgnore = append(output.TagsIgnore, leafString)
}
}
}
}
}
}
}

//TODO: Should these all be migrated over to return an error (it would facilitate better testing) - see this PR:
// https://github.com/nektos/act/pull/1705
if len(output.Branches) > 0 && len(output.BranchesIgnore) > 0 {
log.Fatal("branches and branches-ignore were both specified for the event, but they can't be used simultaneously")
}

if len(output.Paths) > 0 && len(output.PathsIgnore) > 0 {
log.Fatal("paths and paths-ignore were both specified for the event, but they can't be used simultaneously")
}

if len(output.Tags) > 0 && len(output.TagsIgnore) > 0 {
log.Fatal("tags and tags-ignore were both specified for the event, but they can't be used simultaneously")
}

return output
}

//ShouldFilterWorkflow finds any filters relating to eventName in the Workflow definition (e.g. if the eventName is
//"pull_request", there may be a set of "branches" patterns). It then uses the found filters to determine whether the
//workflow should be skipped based on the data in the eventPayload.
func (w *Workflow) ShouldFilterWorkflow(eventName string, ghc GithubContext) bool {
//Find all filter patterns that relate to the input event
filterPatterns := w.FindFilterPatterns(eventName)

if filterPatterns == nil {
return false
}

tw := new(workflowpattern.StdOutTraceWriter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want a logrus tracewriter for consistent logging

it shoud respect --json

Copy link
Author

@ae-ou ae-ou Jul 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ChristopherHX ChristopherHX Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


//Function to build the relevant regex patterns and compare segments of the event payload against them
eventCheckFunc := func(filterFunc workflowpattern.FilterInputsFunc, patterns []string, inputs []string) bool {
regexFilters, err := workflowpattern.CompilePatterns(patterns...)

if err != nil {
log.Fatalf("Failed to convert filter patterns to regex for '%s' workflow: %v", w.File, err)
}

if filterFunc(regexFilters, inputs, tw) {
return true
}

return false
}

//Iterate over the patterns that we call Skip() for (the non *ignore attributes from FilterPatterns)
for _, fp := range [][]string{filterPatterns.Branches, filterPatterns.Paths, filterPatterns.Tags} {
if len(fp) > 0 {
//TODO: pass the relevant data instead of a slice of string - this depends on the GithubContext/event context
if shouldSkip := eventCheckFunc(workflowpattern.Skip, fp, []string{}); shouldSkip {
return true
}
}
}

//Iterate over the *Ignore attributes from the FilterPatterns struct
for _, fp := range [][]string{filterPatterns.BranchesIgnore, filterPatterns.PathsIgnore, filterPatterns.TagsIgnore} {
if len(fp) > 0 {
//TODO: pass the relevant data instead of a slice of string - this depends on the GithubContext/event context
if shouldFilter := eventCheckFunc(workflowpattern.Filter, fp, []string{}); shouldFilter {
return true
}
}
}

return false
}

func (w *Workflow) OnEvent(event string) interface{} {
if w.RawOn.Kind == yaml.MappingNode {
var val map[string]interface{}
Expand Down
Loading