-
-
Notifications
You must be signed in to change notification settings - Fork 354
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
review: Use official qodana github action #4843
Conversation
.github/workflows/tests.yml
Outdated
- uses: github/codeql-action/upload-sarif@3f62b754e23e0dd60f91b744033e1dc1654c0ec6 # tag=v2 | ||
- uses: github/codeql-action/upload-sarif@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove the commit pins. Pinning on tag is not safe as they can move. But most prominently it leads to unexpected failures when devs introduce breaking changes and move tags to those.
if (false==true) { | ||
String s = new String[0].toString(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guessing this is just to trigger an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Co-authored-by: Simon Larsén <[email protected]>
@@ -49,6 +49,9 @@ public ZipFolder(File file) throws IOException { | |||
} | |||
this.file = file; | |||
} | |||
private Object deleteMe() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UnusedMethod: Method 'deleteMe' is never used.
ℹ️ Learn about @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
Command | Usage |
---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]
- Remove deprecated cli options. - set to strict mode failing if more then zero issues are found.
94a0e7d
to
179aa3f
Compare
This ports Qodana to the new style. We fail the CI if any problems are found. If we see this is too strict, we can disable it. |
I'd be happy to merge if you could quickly test whether it really fails the check. There were significant changes since the last time this was tested in this PR, if I did not miss anything. |
@MartinWitt ☝️ conflict :o I've been incredibly busy with other things for the past couple of weeks and feel a bit out of the loop, is there anything keeping us from merging this (except for the conflict)? |
Your guess is as good as mine, but I don't think so. I would really love to get this merged though, as the current pinned hash is now expired and qodana runs fail with:
See https://github.com/INRIA/spoon/actions/runs/3505578524/jobs/5871965639. |
I fixed the conflict. Shall I push some bad to showcase Qodana is working? |
I'd appreciate it :) |
@I-Al-Istannen CI is red as wanted. |
Looks good to me :) |
This reverts commit b3f2b96.
Looks like CI extra checks are failing as snapshots can't be fetched from OW2 repository :/ |
@slarse Fixed now, I think you can review/merge it. Your negative review is greying out the merge button — though I might be able to overrule you. |
@I-Al-Istannen I'd be more comfortable if you merged as you were the last to look at it, I've approved so there shouldn't be any problems now. |
A wise decision, as qodana now failed on master :) EDIT: Looking into github actions and the results here a bit more and checking with #4748, it seems to perform delta checks on PRs and full tests on master. So we probably just need to disable it on master and things should be alright. Neat. |
Jackpot. |
Like ever enterprise project, the developers make horrible decisions at someday and remove the usability.
The Qodana action no longer supports the options we gave it. We need to provide these options as a single string. This is a result of the change in the action using Qodana CLI. Qodana only gave a warning in the log for this behavior, and the last runs since this change could have produced wrong results.
Maybe someone with more advanced YAML skills has a solution for this hideous long string?