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.

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

throw when invalid uses key is provided #1804

Merged
merged 12 commits into from
Jul 11, 2023
36 changes: 27 additions & 9 deletions pkg/model/workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@
func (w *Workflow) OnEvent(event string) interface{} {
if w.RawOn.Kind == yaml.MappingNode {
var val map[string]interface{}
if !decodeNode(w.RawOn, &val) {
return nil

Check warning on line 62 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L61-L62

Added lines #L61 - L62 were not covered by tests
}
return val[event]
}
Expand All @@ -84,14 +84,14 @@
}

var val map[string]yaml.Node
if !decodeNode(w.RawOn, &val) {
return nil

Check warning on line 88 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L87-L88

Added lines #L87 - L88 were not covered by tests
}

var config WorkflowDispatch
node := val["workflow_dispatch"]
if !decodeNode(node, &config) {
return nil

Check warning on line 94 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L93-L94

Added lines #L93 - L94 were not covered by tests
}

return &config
Expand Down Expand Up @@ -120,19 +120,19 @@

func (w *Workflow) WorkflowCallConfig() *WorkflowCall {
if w.RawOn.Kind != yaml.MappingNode {
// The callers expect for "on: workflow_call" and "on: [ workflow_call ]" a non nil return value
return &WorkflowCall{}

Check warning on line 124 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L123-L124

Added lines #L123 - L124 were not covered by tests
}

var val map[string]yaml.Node
if !decodeNode(w.RawOn, &val) {
return &WorkflowCall{}

Check warning on line 129 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L128-L129

Added lines #L128 - L129 were not covered by tests
}

var config WorkflowCall
node := val["workflow_call"]
if !decodeNode(node, &config) {
return &WorkflowCall{}

Check warning on line 135 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L134-L135

Added lines #L134 - L135 were not covered by tests
}

return &config
Expand Down Expand Up @@ -215,8 +215,8 @@
}

var val string
if !decodeNode(j.RawSecrets, &val) {
return false

Check warning on line 219 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L218-L219

Added lines #L218 - L219 were not covered by tests
}

return val == "inherit"
Expand All @@ -228,8 +228,8 @@
}

var val map[string]string
if !decodeNode(j.RawSecrets, &val) {
return nil

Check warning on line 232 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L231-L232

Added lines #L231 - L232 were not covered by tests
}

return val
Expand All @@ -242,12 +242,12 @@
case yaml.ScalarNode:
val = new(ContainerSpec)
if !decodeNode(j.RawContainer, &val.Image) {
return nil

Check warning on line 245 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L245

Added line #L245 was not covered by tests
}
case yaml.MappingNode:
val = new(ContainerSpec)
if !decodeNode(j.RawContainer, val) {
return nil

Check warning on line 250 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L250

Added line #L250 was not covered by tests
}
}
return val
Expand All @@ -258,14 +258,14 @@
switch j.RawNeeds.Kind {
case yaml.ScalarNode:
var val string
if !decodeNode(j.RawNeeds, &val) {
return nil

Check warning on line 262 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L261-L262

Added lines #L261 - L262 were not covered by tests
}
return []string{val}
case yaml.SequenceNode:
var val []string
if !decodeNode(j.RawNeeds, &val) {
return nil

Check warning on line 268 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L267-L268

Added lines #L267 - L268 were not covered by tests
}
return val
}
Expand All @@ -277,14 +277,14 @@
switch j.RawRunsOn.Kind {
case yaml.ScalarNode:
var val string
if !decodeNode(j.RawRunsOn, &val) {
return nil

Check warning on line 281 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L280-L281

Added lines #L280 - L281 were not covered by tests
}
return []string{val}
case yaml.SequenceNode:
var val []string
if !decodeNode(j.RawRunsOn, &val) {
return nil

Check warning on line 287 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L286-L287

Added lines #L286 - L287 were not covered by tests
}
return val
}
Expand All @@ -294,8 +294,8 @@
func environment(yml yaml.Node) map[string]string {
env := make(map[string]string)
if yml.Kind == yaml.MappingNode {
if !decodeNode(yml, &env) {
return nil

Check warning on line 298 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L297-L298

Added lines #L297 - L298 were not covered by tests
}
}
return env
Expand All @@ -311,7 +311,7 @@
if j.Strategy.RawMatrix.Kind == yaml.MappingNode {
var val map[string][]interface{}
if !decodeNode(j.Strategy.RawMatrix, &val) {
return nil

Check warning on line 314 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L314

Added line #L314 was not covered by tests
}
return val
}
Expand Down Expand Up @@ -373,7 +373,7 @@
excludes = append(excludes, e)
} else {
// We fail completely here because that's what GitHub does for non-existing matrix keys, fail on exclude, silent skip on include
return nil, fmt.Errorf("the workflow is not valid. Matrix exclude key %q does not match any key within the matrix", k)

Check warning on line 376 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L376

Added line #L376 was not covered by tests
}
}
}
Expand Down Expand Up @@ -444,14 +444,17 @@
type JobType int

const (
// StepTypeRun is all steps that have a `run` attribute
// JobTypeDefault is all jobs that have a `run` attribute
JobTypeDefault JobType = iota

// StepTypeReusableWorkflowLocal is all steps that have a `uses` that is a local workflow in the .github/workflows directory
// JobTypeReusableWorkflowLocal is all jobs that have a `uses` that is a local workflow in the .github/workflows directory
JobTypeReusableWorkflowLocal

// JobTypeReusableWorkflowRemote is all steps that have a `uses` that references a workflow file in a github repo
// JobTypeReusableWorkflowRemote is all jobs that have a `uses` that references a workflow file in a github repo
JobTypeReusableWorkflowRemote

// JobTypeInvalid represents a job which is not configured correctly
JobTypeInvalid
)

func (j JobType) String() string {
Expand All @@ -467,13 +470,28 @@
}

// Type returns the type of the job
func (j *Job) Type() JobType {
if strings.HasPrefix(j.Uses, "./.github/workflows") && (strings.HasSuffix(j.Uses, ".yml") || strings.HasSuffix(j.Uses, ".yaml")) {
return JobTypeReusableWorkflowLocal
} else if !strings.HasPrefix(j.Uses, "./") && strings.Contains(j.Uses, ".github/workflows") && (strings.Contains(j.Uses, ".yml@") || strings.Contains(j.Uses, ".yaml@")) {
return JobTypeReusableWorkflowRemote
func (j *Job) Type() (JobType, error) {
isReusable := j.Uses != ""

if isReusable {
isYaml, _ := regexp.MatchString(`\.(ya?ml)(?:$|@)`, j.Uses)

if isYaml {
isLocalPath := strings.HasPrefix(j.Uses, "./.github/workflows/")
Copy link
Contributor

Choose a reason for hiding this comment

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

The gitea people probably want ./.gitea/workflows/ to work, but they can just patch it in their fork.

I would just allow any path for reusable workflows regardless of the folder, but this error helps most people using this tool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking it shouldn't matter where the workflow YML is. But when I was using "any path" e.g. under a totally different/custom root dir in the repo, this was causing issues. It's possible that it was actually find and the issue was something else.

I'll push a change for this.

isRemotePath := strings.Contains(j.Uses, "/.github/workflows/")
hasVersion, _ := regexp.MatchString(`\.ya?ml@`, j.Uses)

if isLocalPath {
return JobTypeReusableWorkflowLocal, nil
} else if isRemotePath && hasVersion {
return JobTypeReusableWorkflowRemote, nil
}
}

return JobTypeInvalid, fmt.Errorf("`uses` key references invalid workflow path '%s'. Must start with './.github/workflows/' if it's a local workflow, or must start with '<org>/<repo>/.github/workflows/' and include an '@' if it's a remote workflow", j.Uses)
}
return JobTypeDefault

return JobTypeDefault, nil
}

// ContainerSpec is the specification of the container to use for the job
Expand Down Expand Up @@ -659,16 +677,16 @@
return ids
}

var OnDecodeNodeError = func(node yaml.Node, out interface{}, err error) {
log.Fatalf("Failed to decode node %v into %T: %v", node, out, err)

Check warning on line 681 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L680-L681

Added lines #L680 - L681 were not covered by tests
}

func decodeNode(node yaml.Node, out interface{}) bool {
if err := node.Decode(out); err != nil {
if OnDecodeNodeError != nil {
OnDecodeNodeError(node, out, err)
}
return false

Check warning on line 689 in pkg/model/workflow.go

View check run for this annotation

Codecov / codecov/patch

pkg/model/workflow.go#L686-L689

Added lines #L686 - L689 were not covered by tests
}
return true
}
79 changes: 71 additions & 8 deletions pkg/model/workflow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ jobs:
})
}

//nolint:dupl
func TestReadWorkflow_JobTypes(t *testing.T) {
yaml := `
name: invalid job definition
Expand All @@ -147,20 +148,82 @@ jobs:
runs-on: ubuntu-latest
steps:
- run: echo
remote-reusable-workflow:
runs-on: ubuntu-latest
remote-reusable-workflow-yml:
uses: remote/repo/.github/workflows/workflow.yml@main
local-reusable-workflow:
runs-on: ubuntu-latest
remote-reusable-workflow-yaml:
uses: remote/repo/.github/workflows/workflow.yaml@main
local-reusable-workflow-yml:
uses: ./.github/workflows/workflow.yml
local-reusable-workflow-yaml:
uses: ./.github/workflows/workflow.yaml
`

workflow, err := ReadWorkflow(strings.NewReader(yaml))
assert.NoError(t, err, "read workflow should succeed")
assert.Len(t, workflow.Jobs, 5)

job, err := workflow.Jobs["default-job"].Type()
assert.Equal(t, nil, err)
assert.Equal(t, JobTypeDefault, job)

job, err = workflow.Jobs["remote-reusable-workflow-yml"].Type()
assert.Equal(t, nil, err)
assert.Equal(t, JobTypeReusableWorkflowRemote, job)

job, err = workflow.Jobs["remote-reusable-workflow-yaml"].Type()
assert.Equal(t, nil, err)
assert.Equal(t, JobTypeReusableWorkflowRemote, job)

job, err = workflow.Jobs["local-reusable-workflow-yml"].Type()
assert.Equal(t, nil, err)
assert.Equal(t, JobTypeReusableWorkflowLocal, job)

job, err = workflow.Jobs["local-reusable-workflow-yaml"].Type()
assert.Equal(t, nil, err)
assert.Equal(t, JobTypeReusableWorkflowLocal, job)
}

//nolint:dupl
func TestReadWorkflow_JobTypes_InvalidPath(t *testing.T) {
yaml := `
name: invalid job definition

jobs:
remote-reusable-workflow-missing-version:
uses: remote/repo/.github/workflows/workflow.yml
remote-reusable-workflow-bad-extension:
uses: remote/repo/.github/workflows/workflow.json
remote-reusable-workflow-bad-path:
uses: remote/repo/github/workflows/workflow.yaml@main
local-reusable-workflow-bad-extension:
uses: ./.github/workflows/workflow.json
local-reusable-workflow-bad-path:
uses: ./.github/workflow/workflow.yaml
`

workflow, err := ReadWorkflow(strings.NewReader(yaml))
assert.NoError(t, err, "read workflow should succeed")
assert.Len(t, workflow.Jobs, 3)
assert.Equal(t, workflow.Jobs["default-job"].Type(), JobTypeDefault)
assert.Equal(t, workflow.Jobs["remote-reusable-workflow"].Type(), JobTypeReusableWorkflowRemote)
assert.Equal(t, workflow.Jobs["local-reusable-workflow"].Type(), JobTypeReusableWorkflowLocal)
assert.Len(t, workflow.Jobs, 5)

job, err := workflow.Jobs["remote-reusable-workflow-missing-version"].Type()
assert.Equal(t, JobTypeInvalid, job)
assert.NotEqual(t, nil, err)

job, err = workflow.Jobs["remote-reusable-workflow-bad-extension"].Type()
assert.Equal(t, JobTypeInvalid, job)
assert.NotEqual(t, nil, err)

job, err = workflow.Jobs["remote-reusable-workflow-bad-path"].Type()
assert.Equal(t, JobTypeInvalid, job)
assert.NotEqual(t, nil, err)

job, err = workflow.Jobs["local-reusable-workflow-bad-extension"].Type()
assert.Equal(t, JobTypeInvalid, job)
assert.NotEqual(t, nil, err)

job, err = workflow.Jobs["local-reusable-workflow-bad-path"].Type()
assert.Equal(t, JobTypeInvalid, job)
assert.NotEqual(t, nil, err)
}

func TestReadWorkflow_StepsTypes(t *testing.T) {
Expand Down
22 changes: 15 additions & 7 deletions pkg/runner/run_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,18 +89,18 @@

func getDockerDaemonSocketMountPath(daemonPath string) string {
if protoIndex := strings.Index(daemonPath, "://"); protoIndex != -1 {
scheme := daemonPath[:protoIndex]
if strings.EqualFold(scheme, "npipe") {
// linux container mount on windows, use the default socket path of the VM / wsl2
return "/var/run/docker.sock"
} else if strings.EqualFold(scheme, "unix") {
return daemonPath[protoIndex+3:]
} else if strings.IndexFunc(scheme, func(r rune) bool {
return (r < 'a' || r > 'z') && (r < 'A' || r > 'Z')
}) == -1 {
// unknown protocol use default
return "/var/run/docker.sock"
}

Check warning on line 103 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L92-L103

Added lines #L92 - L103 were not covered by tests
}
return daemonPath
}
Expand Down Expand Up @@ -319,11 +319,11 @@
if rc.ExtraPath != nil && len(rc.ExtraPath) > 0 {
path := rc.JobContainer.GetPathVariableName()
if rc.JobContainer.IsEnvironmentCaseInsensitive() {
// On windows system Path and PATH could also be in the map
for k := range *env {
if strings.EqualFold(path, k) {
path = k
break

Check warning on line 326 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L322-L326

Added lines #L322 - L326 were not covered by tests
}
}
}
Expand All @@ -336,8 +336,8 @@
}
}
if len(cpath) == 0 {
cpath = rc.JobContainer.DefaultPathVariable()
}

Check warning on line 340 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L339-L340

Added lines #L339 - L340 were not covered by tests
(*env)[path] = cpath
}
(*env)[path] = rc.JobContainer.JoinPathVariable(append(rc.ExtraPath, (*env)[path])...)
Expand Down Expand Up @@ -389,8 +389,8 @@
if home, err := os.UserHomeDir(); err == nil {
xdgCache = filepath.Join(home, ".cache")
} else if xdgCache, err = filepath.Abs("."); err != nil {
// It's almost impossible to get here, so the temp dir is a good fallback
xdgCache = os.TempDir()

Check warning on line 393 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L392-L393

Added lines #L392 - L393 were not covered by tests
}
}
return filepath.Join(xdgCache, "act")
Expand Down Expand Up @@ -451,16 +451,19 @@
}

// Executor returns a pipeline executor for all the steps in the job
func (rc *RunContext) Executor() common.Executor {
func (rc *RunContext) Executor() (common.Executor, error) {
var executor common.Executor
var jobType, err = rc.Run.Job().Type()

switch rc.Run.Job().Type() {
switch jobType {
case model.JobTypeDefault:
executor = newJobExecutor(rc, &stepFactoryImpl{}, rc)
case model.JobTypeReusableWorkflowLocal:
executor = newLocalReusableWorkflowExecutor(rc)
case model.JobTypeReusableWorkflowRemote:
executor = newRemoteReusableWorkflowExecutor(rc)
case model.JobTypeInvalid:
return nil, err

Check warning on line 466 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L465-L466

Added lines #L465 - L466 were not covered by tests
}

return func(ctx context.Context) error {
Expand All @@ -472,7 +475,7 @@
return executor(ctx)
}
return nil
}
}, nil
}

func (rc *RunContext) containerImage(ctx context.Context) string {
Expand Down Expand Up @@ -525,16 +528,21 @@
func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) {
job := rc.Run.Job()
l := common.Logger(ctx)
runJob, err := EvalBool(ctx, rc.ExprEval, job.If.Value, exprparser.DefaultStatusCheckSuccess)
if err != nil {
return false, fmt.Errorf(" \u274C Error in if-expression: \"if: %s\" (%s)", job.If.Value, err)
runJob, runJobErr := EvalBool(ctx, rc.ExprEval, job.If.Value, exprparser.DefaultStatusCheckSuccess)
jobType, jobTypeErr := job.Type()

if runJobErr != nil {
return false, fmt.Errorf(" \u274C Error in if-expression: \"if: %s\" (%s)", job.If.Value, runJobErr)

Check warning on line 535 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L535

Added line #L535 was not covered by tests
}

if !runJob {
l.WithField("jobResult", "skipped").Debugf("Skipping job '%s' due to '%s'", job.Name, job.If.Value)
return false, nil
}

if job.Type() != model.JobTypeDefault {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops the else if jobType != model.JobTypeDefault { block needs to be moved back, because it should allow skipping reusable workflows.

While jobtypeinvalid should return an error as early as possible.

This means we don't have a test for skipping reusable workflows

if jobType == model.JobTypeInvalid {
return false, jobTypeErr

Check warning on line 544 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L544

Added line #L544 was not covered by tests
} else if jobType != model.JobTypeDefault {
JoshMcCullough marked this conversation as resolved.
Show resolved Hide resolved
return true, nil
}

Expand Down Expand Up @@ -687,13 +695,13 @@
}
// allow to be overridden by user
if rc.Config.Env["GITHUB_SERVER_URL"] != "" {
ghc.ServerURL = rc.Config.Env["GITHUB_SERVER_URL"]
}

Check warning on line 699 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L698-L699

Added lines #L698 - L699 were not covered by tests
if rc.Config.Env["GITHUB_API_URL"] != "" {
ghc.APIURL = rc.Config.Env["GITHUB_API_URL"]
}

Check warning on line 702 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L701-L702

Added lines #L701 - L702 were not covered by tests
if rc.Config.Env["GITHUB_GRAPHQL_URL"] != "" {
ghc.GraphQLURL = rc.Config.Env["GITHUB_GRAPHQL_URL"]

Check warning on line 704 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L704

Added line #L704 was not covered by tests
}

return ghc
Expand Down Expand Up @@ -799,7 +807,7 @@
func setActionRuntimeVars(rc *RunContext, env map[string]string) {
actionsRuntimeURL := os.Getenv("ACTIONS_RUNTIME_URL")
if actionsRuntimeURL == "" {
actionsRuntimeURL = fmt.Sprintf("http://%s:%s/", rc.Config.ArtifactServerAddr, rc.Config.ArtifactServerPort)

Check warning on line 810 in pkg/runner/run_context.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/run_context.go#L810

Added line #L810 was not covered by tests
}
env["ACTIONS_RUNTIME_URL"] = actionsRuntimeURL

Expand Down
8 changes: 7 additions & 1 deletion pkg/runner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@
}
eventJSON, err := json.Marshal(eventMap)
if err != nil {
return nil, err
}

Check warning on line 95 in pkg/runner/runner.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/runner.go#L94-L95

Added lines #L94 - L95 were not covered by tests
runner.eventJSON = string(eventJSON)
}
return runner, nil
Expand Down Expand Up @@ -120,7 +120,7 @@

var matrixes []map[string]interface{}
if m, err := job.GetMatrixes(); err != nil {
log.Errorf("Error while get job's matrix: %v", err)

Check warning on line 123 in pkg/runner/runner.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/runner.go#L123

Added line #L123 was not covered by tests
} else {
matrixes = selectMatrixes(m, runner.config.Matrix)
}
Expand All @@ -147,7 +147,13 @@
}
stageExecutor = append(stageExecutor, func(ctx context.Context) error {
jobName := fmt.Sprintf("%-*s", maxJobNameLen, rc.String())
return rc.Executor()(common.WithJobErrorContainer(WithJobLogger(ctx, rc.Run.JobID, jobName, rc.Config, &rc.Masks, matrix)))
executor, err := rc.Executor()

if err != nil {
return err
}

Check warning on line 154 in pkg/runner/runner.go

View check run for this annotation

Codecov / codecov/patch

pkg/runner/runner.go#L153-L154

Added lines #L153 - L154 were not covered by tests

return executor(common.WithJobErrorContainer(WithJobLogger(ctx, rc.Run.JobID, jobName, rc.Config, &rc.Masks, matrix)))
})
}
pipeline = append(pipeline, common.NewParallelExecutor(maxParallel, stageExecutor...))
Expand Down