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

[Merged by Bors] - feat(ci): Emit error messages in a way understood by github #5726

Closed
wants to merge 12 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jan 13, 2021

This uses the commands described here, for which the implementation provides a slightly clearer spec.
This means github now annotates broken lines, and highlights the error in red.

Originally I tried to implement this using "problem matchers", but these do not support multi-line error messages.

Supporting this in the linter is something that I'll leave for a follow-up PR.


Click the "files changed" tab to see the automated failure comment added by github.

image

The "checks tab" shows

image

The downside is that the plaintext output no longer shows filename / line numbers:

image

@eric-wieser eric-wieser changed the title feat(ci): try out GitHub Matchers feat(ci): Emit error messages in a way understood by github Jan 13, 2021
@eric-wieser eric-wieser marked this pull request as ready for review January 13, 2021 18:05
@eric-wieser eric-wieser added awaiting-review The author would like community review of the PR CI This issue or PR is about continuous integration labels Jan 13, 2021
Copy link
Collaborator

@bryangingechen bryangingechen left a comment

Choose a reason for hiding this comment

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

Very neat!

Note that detect_errors.py is used in a few other projects: e.g. https://github.com/leanprover-community/azure-scripts/blob/master/.github/workflows/nolints.yml and https://github.com/leanprover-community/lean-web-editor/blob/master/.github/workflows/build.yml

Do you happen to know how GitHub will treat build errors in those projects?

scripts/detect_errors.py Show resolved Hide resolved
@eric-wieser
Copy link
Member Author

eric-wieser commented Jan 13, 2021

Do you happen to know how GitHub will treat build errors in those projects?

No, but I'd be surprised if it does something different to what is happening in this one. It's probably a good thing I didn't use the separate problem-matcher json file, as it wouldn't have found that.

scripts/detect_errors.py Outdated Show resolved Hide resolved
@eric-wieser
Copy link
Member Author

Note that detect_errors.py is used in a few other projects:

This one is a different file with the same name.

@bryangingechen
Copy link
Collaborator

Note that detect_errors.py is used in a few other projects:

This one is a different file with the same name.

🤦 Ah, my bad.

eric-wieser and others added 2 commits January 13, 2021 18:52
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@bryangingechen
Copy link
Collaborator

OK, let's try it out. Thanks!
bors r+

@github-actions github-actions bot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Jan 14, 2021
bors bot pushed a commit that referenced this pull request Jan 14, 2021
This uses the commands described [here](https://github.com/actions/toolkit/blob/master/docs/commands.md#log-level), for which [the implementation](https://github.com/actions/toolkit/blob/af821474235d3c5e1f49cee7c6cf636abb0874c4/packages/core/src/command.ts#L36-L94) provides a slightly clearer spec.
This means github now annotates broken lines, and highlights the error in red.

Originally I tried to implement this using "problem matchers", but these do not support multi-line error messages.

Supporting this in the linter is something that I'll leave for a follow-up PR.



Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
@bors
Copy link

bors bot commented Jan 14, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(ci): Emit error messages in a way understood by github [Merged by Bors] - feat(ci): Emit error messages in a way understood by github Jan 14, 2021
@bors bors bot closed this Jan 14, 2021
@bors bors bot deleted the github-matchers branch January 14, 2021 05:24
@eric-wieser
Copy link
Member Author

Looks like this even works when the error is in a file not changed in the PR:

image

b-mehta pushed a commit that referenced this pull request Jan 16, 2021
This uses the commands described [here](https://github.com/actions/toolkit/blob/master/docs/commands.md#log-level), for which [the implementation](https://github.com/actions/toolkit/blob/af821474235d3c5e1f49cee7c6cf636abb0874c4/packages/core/src/command.ts#L36-L94) provides a slightly clearer spec.
This means github now annotates broken lines, and highlights the error in red.

Originally I tried to implement this using "problem matchers", but these do not support multi-line error messages.

Supporting this in the linter is something that I'll leave for a follow-up PR.



Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI This issue or PR is about continuous integration ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants