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

Add compilation errors & SAT GitHub annotations #2543

Merged
merged 14 commits into from Nov 13, 2021
Merged

Conversation

ghys
Copy link
Member

@ghys ghys commented Oct 27, 2021

No description provided.

Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
@ghys ghys requested a review from a team as a code owner October 27, 2021 17:36
Signed-off-by: Yannick Schaus <github@schaus.net>
@ghys
Copy link
Member Author

ghys commented Oct 27, 2021

Sorry I meant to open this PR in my fork 😠

Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
@ghys
Copy link
Member Author

ghys commented Oct 28, 2021

So this is basically working (see the Files tab) with the following caveats:

What can be done is:

  • fork the action to filter out the warnings and keep only the errors - this needs a fork because it's not offered as an option but should be easy to do
  • fix the obvious errors as reported here ("There should be no empty lines between tags in a Javadoc comment of a method or constructor.")

Then the inline problem reports should be less noisy and only report what the user needs to fix to make the build pass.

Wdyt? @wborn

@wborn
Copy link
Member

wborn commented Oct 28, 2021

Thanks for having a look at this. 👍 I like the result and would also rather avoid having the "Unchanged files with check permissions" section show. Showing errors only would probably make the most sense now. The "10 errors & 10 warnings" is good enough for me.

I read that another issue is that the line numbers won't align if you use the files resulting from the merged branch. So that's why the example uses ref: ${{ github.event.pull_request.head.sha }}. But I would really want the code to be build using the merged branch so we know for sure there won't be any issues when merging PRs. Branches could be based on very old code too causing all sorts of issues. So then it is probably better to create another workflow for adding the annotations.

@wborn
Copy link
Member

wborn commented Oct 30, 2021

Many static analysis tools nowadays also support the SARIF format.

It seems you can also use these files to integrate with the GitHub code scanning, see:

https://docs.github.com/en/code-security/code-scanning
https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/about-integration-with-code-scanning

Might also be worth to look into what this exactly does and how well it works when PRs are created. 😉

Remove deliberate error
Remove other errors found in last run

Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
@ghys
Copy link
Member Author

ghys commented Nov 3, 2021

So in the end I combined these two PRs into one:

  • one that reports & displays Java compilation errors as annotations (example)
  • one that reports SAT errors as annotations (example)

(Try Ctrl-F5 if the annotations don't show)

I eventually fixed some unrelated problems in this PR, that's because the action originally had a bug and was treating every problem as errors unless there were warnings - so info levels were treated as failures... If they are not wanted I can revert them.

@ghys
Copy link
Member Author

ghys commented Nov 3, 2021

https://docs.github.com/en/code-security/code-scanning
https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/about-integration-with-code-scanning

Those are interesting as well but it might be better to postpone them to another workflow dedicated to code checks?

@ghys ghys changed the title Add Spotbugs & Checkstyle annotation actions Add compilation errors & SAT GitHub annotations Nov 3, 2021
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks a lot. 👍
I gave it a brief test in my own repo and it seems to work. 🙂
So let's test how it works for real!

@wborn wborn merged commit d4f62ed into openhab:main Nov 13, 2021
@wborn wborn added this to the 3.2 milestone Nov 13, 2021
@ghys
Copy link
Member Author

ghys commented Nov 14, 2021

If you think it'd be a good addition to other repos (notably openhab-addons) I can open similar PRs to those too.

@wborn
Copy link
Member

wborn commented Nov 14, 2021

Yes it would be nice to test it in the add-ons repo too!

I plan on adding GHA to more repos when all bugs are sorted out and we're happy with the features in the most active repos. 🙂 That prevents a lot of PRs to tune GHA everywhere. It might also be worth looking into reusing workflows if we add GHA to many repos.

wborn added a commit to wborn/openhab-addons that referenced this pull request Nov 28, 2021
Let's test the GHA annotations for errors also in this repo!

Similar to openhab/openhab-core#2543

Signed-off-by: Wouter Born <github@maindrain.net>
fwolter pushed a commit to openhab/openhab-addons that referenced this pull request Nov 28, 2021
Let's test the GHA annotations for errors also in this repo!

Similar to openhab/openhab-core#2543

Signed-off-by: Wouter Born <github@maindrain.net>
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Dec 30, 2021
Let's test the GHA annotations for errors also in this repo!

Similar to openhab/openhab-core#2543

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
mischmidt83 pushed a commit to mischmidt83/openhab-addons that referenced this pull request Jan 9, 2022
Let's test the GHA annotations for errors also in this repo!

Similar to openhab/openhab-core#2543

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Michael Schmidt <mi.schmidt.83@gmail.com>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jan 28, 2022
Let's test the GHA annotations for errors also in this repo!

Similar to openhab/openhab-core#2543

Signed-off-by: Wouter Born <github@maindrain.net>
volkmarnissen pushed a commit to volkmarnissen/openhab-addons that referenced this pull request Mar 3, 2022
Let's test the GHA annotations for errors also in this repo!

Similar to openhab/openhab-core#2543

Signed-off-by: Wouter Born <github@maindrain.net>
marcfischerboschio pushed a commit to marcfischerboschio/openHABaddon that referenced this pull request Apr 20, 2022
Let's test the GHA annotations for errors also in this repo!

Similar to openhab/openhab-core#2543

Signed-off-by: Wouter Born <github@maindrain.net>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
Let's test the GHA annotations for errors also in this repo!

Similar to openhab/openhab-core#2543

Signed-off-by: Wouter Born <github@maindrain.net>
@vivox80pro
Copy link

#16

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
Signed-off-by: Yannick Schaus <github@schaus.net>
GitOrigin-RevId: d4f62ed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants