-
-
Notifications
You must be signed in to change notification settings - Fork 451
Run CodeQL only post merge #4385
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
Conversation
Performance metrics 🚀
|
@@ -3,9 +3,6 @@ name: 'CodeQL' | |||
on: | |||
push: | |||
branches: [main] | |||
pull_request: |
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.
To be honest I would still run CodeQL for PRs against main, as it will block the PR from being merged if there are CodeQL 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.
I think it's probably fine to run it on main only, because I don't remember a single time where it failed for a valid reason 😅 We'll still get the notification if it's failed on main and see the results here: https://github.com/getsentry/sentry-java/security/code-scanning
The reason we don't want to run it on PRs because without build-cache it'd be the longest one to run. Alternatively we could skip the run altogether, if there were no code changes to prevent the job from failing, but I think it's a good middle-ground for now.
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.
LGTM, let's see how it goes and we add a skip-check later, if we see it necessary 👍
📜 Description
This runs CodeQL with build caching disabled post-merge.
#skip-changelog
💡 Motivation and Context
CodeQL is slow and requires the compilation tasks to re-run. By disabling build caching we can ensure that CodeQL is run correctly. However since this is one of the longest checks, it doesn’t make sense to run it during PRs.
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps