Now I’ve hit a roadblock, namely that check_suite.(re)requested jaime-ci-bot/index.js at b506954c4f19b78c3b7f548324d79179daf6b640 · MeFisto94/jaime-ci-bot · GitHub) is not called for foreign PRs, only for local commits and local PRs.
This is done for security purposes, I guess, but github actions definitely creates a check_suite, which we’d love to hook into. Especially because a) it runs whenever a new commit is happening (or a force push), which rules out the pull_requests events and b) because I need the sha of the first commit of the PR and the sha of the latest commit to checkout the PR.
On a sidenote, we’ve found a third-party/commercial alternative to what we do, which is codacy.
There might be alternatives to codacy so if you can either comment on codacy or have a different provider in mind, please tell us.
Given the much increased feature set compared to what we had, it might be worth considering to use Codacy. It even has support to lint markdown files, and I guess we could also enforce formatting that way, without having to add significant code to the bot otherwise.
The downside is that Codacy does not have “diffing”, which means ever lasting errors of jme might be visible for each new PR, like we need to whitelist false positives, which might be possible.
What are your opinions?
I’d love to finish this ASAP so we can start our “spring cleaning” finally to catch up with what I have in the jme queue.
Take a look at PR 1406. One of the failing tests is labelled “Codacy/PR Quality Review — Not up to standards. This pull request quality could be better.”
Well then that’d be my suggestion, let’s try it out via the Spring Cleaning that I wanted to do anyway, to see how it performs.
It’s analytics also found some things spotbugs could not catch as it applies to native code.
I guess if we can set it up properly to respect formatting and others we get the most value out of it.
@sgold I still have to check the PR, but I guess the “misunderstanding” here is that the fail is only a note to the reviewers, the PR can still be merged
Edit2: It also shows what we might need to configure, it enforces static methods to be camelCase and does not like the underscores in the native methods here.
It should be completely disabled for PRs now.
You can log in into the codacy organization with your github account and be automatically added to the admins, i think, if it doesn’t work, let me know, i’ll look into doing it manually.
There are some configurations to whitelist “issues” and set the sensitivity/type of analysis, we should look into configuring that.
It is also possible to integrate spotbug running externally (github actions?).
Yeah, jmonkeyengine - Codacy - Files is a good example, it did not detect that we just pass “0” and make it fail in Line 38, it just expects an if around the recursive call.
But then again it’s a good thing it would even catch such cases, in native code.
I’ll look into configuring a bit, but I guess we can’t properly tell until seeing a lot of code/PRs and discussion (e.g. if it was for me, your PRs all need reformatting and optimization of imports etc )
)