reviewdog icon indicating copy to clipboard operation
reviewdog copied to clipboard

Error code 2 results in GitHub Action success

Open TomasBarry opened this issue 5 years ago • 3 comments

Our GitHub Action is making use of ReviewDog to run ESLint on the diff in a PR:

- name: Run ESlint
  run: |
    diff_sha=$(git rev-parse "origin/${{ github.base_ref }}")
    yarn run eslint app/javascript | ./bin/reviewdog -f=eslint -diff="git diff $diff_sha" -reporter=github-pr-check

However, the result of this action is an exit code of 2 but ReviewDog still marks the step with a conclusion of 'success':

Warning: React version not specified in eslint-plugin-react settings. See https://github.com/yannickcr/eslint-plugin-react#configuration .
TypeError: Cannot read property 'elements' of undefined
Occurred while linting /home/runner/work/***/***/app/javascript/cms/components/RichTextSection.spec.js:16
    at CallExpression (/home/runner/work/***/***/node_modules/eslint-plugin-react/lib/rules/no-adjacent-inline-elements.js:110:44)
    at listeners.(anonymous function).forEach.listener (/home/runner/work/***/***/node_modules/eslint/lib/linter/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/home/runner/work/***/***/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/home/runner/work/***/***/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/home/runner/work/***/***/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/home/runner/work/***/***/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (/home/runner/work/***/***/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:634:23)
    at nodeQueue.forEach.traversalInfo (/home/runner/work/***/***/node_modules/eslint/lib/linter/linter.js:936:32)
    at Array.forEach (<anonymous>)
error Command failed with exit code 2.
2020/08/18 13:34:19 [eslint] reported: https://github.com/***/***/runs/12345678 (conclusion=success)

The important lines are the last two:

error Command failed with exit code 2.
2020/08/18 13:34:19 [eslint] reported: https://github.com/***/***/runs/12345678 (conclusion=success)

We've tracked down the cause of the error that causes ESLint to fail. However, I felt that this issue was best raised with ReviewDog as an exit code of 2 resulting in the action being given a conclusion of 'success' does not seem correct.

My suspicion here is that the status code should be checked as well where you assign the conclusion in doghouse/server/doghouse.go. Perhaps it is that the Context should be checked too?

func (ch *Checker) postCheck(ctx context.Context, checkID int64, checks []*filter.FilteredDiagnostic) (*github.CheckRun, string, error) {
	var annotations []*github.CheckRunAnnotation
	for _, c := range checks {
		if !c.ShouldReport {
			continue
		}
		annotations = append(annotations, ch.toCheckRunAnnotation(c))
	}
	if len(annotations) > 0 {
		if err := ch.postAnnotations(ctx, checkID, annotations); err != nil {
			return nil, "", fmt.Errorf("failed to post annotations: %w", err)
		}
	}

	conclusion := "success" // Something with ctx perhaps?
	if len(annotations) > 0 {
		conclusion = ch.conclusion()
	}
	opt := github.UpdateCheckRunOptions{
		Name:        ch.checkName(),
		Status:      github.String("completed"),
		Conclusion:  github.String(conclusion),
		CompletedAt: &github.Timestamp{Time: time.Now()},
		Output: &github.CheckRunOutput{
			Title:   github.String(ch.checkTitle()),
			Summary: github.String(ch.summary(checks)),
		},
	}
	checkRun, err := ch.gh.UpdateCheckRun(ctx, ch.req.Owner, ch.req.Repo, checkID, opt)
	if err != nil {
		return nil, "", err
	}
	return checkRun, conclusion, nil
}

TomasBarry avatar Aug 18 '20 14:08 TomasBarry

@haya14busa (or any other maintainer) - any views on this?

TomasBarry avatar Sep 21 '20 09:09 TomasBarry

@TomasBarry did you find a solution for this?

mrousavy avatar Mar 12 '21 11:03 mrousavy

@mrousavy - unfortunately not.

TomasBarry avatar Mar 12 '21 13:03 TomasBarry