Skip to content

Commit

Permalink
throw when invalid uses key is provided (#1804)
Browse files Browse the repository at this point in the history
* throw if `uses` is invalid

* update JobType to return error

* lint

* put //nolint:dupl on wrong test

* update error message to remove end punctuation

* lint

* update remote job type check

* move if statement

* rm nolint:dupl ... we'll see how that goes

---------

Co-authored-by: Casey Lee <cplee@nektos.com>
  • Loading branch information
JoshMcCullough and cplee authored Jul 11, 2023
1 parent 8c7c0f5 commit 808cf5a
Show file tree
Hide file tree
Showing 4 changed files with 123 additions and 30 deletions.
36 changes: 27 additions & 9 deletions pkg/model/workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -445,14 +445,17 @@ func commonKeysMatch2(a map[string]interface{}, b map[string]interface{}, m map[
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 @@ -468,13 +471,28 @@ func (j JobType) String() string {
}

// 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, "./")
isRemotePath, _ := regexp.MatchString(`^[^.](.+?/){2,}.+\.ya?ml@`, j.Uses)
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 './' if it's a local workflow, or must start with '<org>/<repo>/' 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
81 changes: 71 additions & 10 deletions pkg/model/workflow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,20 +147,81 @@ jobs:
runs-on: ubuntu-latest
steps:
- run: echo
remote-reusable-workflow:
runs-on: ubuntu-latest
uses: remote/repo/.github/workflows/workflow.yml@main
local-reusable-workflow:
runs-on: ubuntu-latest
uses: ./.github/workflows/workflow.yml
remote-reusable-workflow-yml:
uses: remote/repo/some/path/to/workflow.yml@main
remote-reusable-workflow-yaml:
uses: remote/repo/some/path/to/workflow.yaml@main
remote-reusable-workflow-custom-path:
uses: remote/repo/path/to/workflow.yml@main
local-reusable-workflow-yml:
uses: ./some/path/to/workflow.yml
local-reusable-workflow-yaml:
uses: ./some/path/to/workflow.yaml
`

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

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

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

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

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

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

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

func TestReadWorkflow_JobTypes_InvalidPath(t *testing.T) {
yaml := `
name: invalid job definition
jobs:
remote-reusable-workflow-missing-version:
uses: remote/repo/some/path/to/workflow.yml
remote-reusable-workflow-bad-extension:
uses: remote/repo/some/path/to/workflow.json
local-reusable-workflow-bad-extension:
uses: ./some/path/to/workflow.json
local-reusable-workflow-bad-path:
uses: some/path/to/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, 4)

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

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

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

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

func TestReadWorkflow_StepsTypes(t *testing.T) {
Expand Down
28 changes: 18 additions & 10 deletions pkg/runner/run_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -454,16 +454,19 @@ func (rc *RunContext) steps() []*model.Step {
}

// 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
}

return func(ctx context.Context) error {
Expand All @@ -475,7 +478,7 @@ func (rc *RunContext) Executor() common.Executor {
return executor(ctx)
}
return nil
}
}, nil
}

func (rc *RunContext) containerImage(ctx context.Context) string {
Expand Down Expand Up @@ -528,19 +531,24 @@ func (rc *RunContext) options(_ context.Context) string {
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)
}

if jobType == model.JobTypeInvalid {
return false, jobTypeErr
} else if jobType != model.JobTypeDefault {
return true, nil
}

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 {
return true, nil
}

img := rc.platformImage(ctx)
if img == "" {
if job.RunsOn() == nil {
Expand Down
8 changes: 7 additions & 1 deletion pkg/runner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,13 @@ func (runner *runnerImpl) NewPlanExecutor(plan *model.Plan) common.Executor {
}
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
}

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

0 comments on commit 808cf5a

Please sign in to comment.