-
Notifications
You must be signed in to change notification settings - Fork 7
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 CodeQL and DeepSource support #123
Conversation
For some reason this was set up to only (I think) run if there was a push onto `main`, which basically never happens directly because `main` is protected. I also changed the cron from running once a week to running once a month. Given that we're not really worried about security issues on this repo, and it sits dormant for months at a stretch, it seemed to make more sense to have it run less often.
This sets the runtime to be v21 of Java, which is what we're using. Their docs (https://docs.deepsource.com/docs/analyzers-java#meta) suggest that they only support up to v19, though, so I'm not sure what will happen here.
I think this got CodeQL working, but DeepSource remains silent. That may require doing something on their web site, and I think that @kklamberty needs to add me to a team on the DeepSource site for me to be able to do that. |
SonarCloud recommended (quoting the JUnit5 User Guide) making the test classes and methods package visible, which just involves removing the `public` in front of their declarations.
SonarCloud (and JUnit5 best practices) suggest that test classes should be package visible. Unfortunately that causes CodeQL to complain that we have an unused type, which is a potential security problem. I'm trying to resolve that by suppressing the `unused-reference-type` warning, but I don't know if that will actually work. We'll see.
This reverts commit 8d76375.
I've emailed an invite to @NicMcPhee to be an administrator, and I also went ahead and added the repository. I was unable to activate it, though, since the toml file is not yet in the main branch (as shown here). I think that means once it's merged you still may need to activate it manually. I have left it as a setting to manually add repositories since we have quite a few repos that I don't think we will want activated. |
It does seem like this is causing troubles. The DeepSource check says we have errors in the |
We're really (at the moment) still using v21, but we're not using any v21 features, so I think this should be "safe". Hopefully it will fix the errors we're getting from DeepSource about an incorrect config file.
DeepSource (correctly) complained about our use of raw `for` loops, where SonarQube has remained silent on this. I've added a bogus test that uses a (new) raw `for` loop to see if that causes SonarQube to say something.
This reverts commit 853da52.
This addresses a warning from DeepSource about the fact that we were using a "raw" `for` loop when we should be using a `for-each` style loop. I had hoped that I could also replace the other `for` loop, but that can't be easily done since we're looping over two parallel arrays and need the index for the error message.
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
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.
Looks good!
This hopefully gets both CodeQL running (better) and DeepSource working (for the first time on this repo).
This sets the DeepSource runtime to be v17 of Java, as that's the most recent LTS version they support (at the moment). We actually using v21, but there's nothing in this project that uses fancy new features of Java, so we should be safe until DeepSource gets up to supporting v21.