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

Apply CodeQL to Every Branch #185

Closed
ArielSAdamsNASA opened this issue Feb 3, 2021 · 8 comments · Fixed by #187
Closed

Apply CodeQL to Every Branch #185

ArielSAdamsNASA opened this issue Feb 3, 2021 · 8 comments · Fixed by #187

Comments

@ArielSAdamsNASA
Copy link
Contributor

ArielSAdamsNASA commented Feb 3, 2021

Describe the bug
CodeQL only scans the main branch on push and pull-request.

Expected behavior
Scan every branch on push and pull-request using CodeQL.

Reporter Info
Ariel Adams, ASRC Federal

@ArielSAdamsNASA ArielSAdamsNASA self-assigned this Feb 3, 2021
@ArielSAdamsNASA ArielSAdamsNASA added the CCB:Ignore Pull Request is NOT ready for discussion. Has open actions. Will be re-examined at by next CCB. label Feb 3, 2021
@skliper
Copy link
Contributor

skliper commented Feb 4, 2021

I recommend scanning on every branch (push and pull-request) for now.

@ArielSAdamsNASA ArielSAdamsNASA changed the title Apply CodeQL to Integration Candidate branch Apply CodeQL to Every Branch Feb 4, 2021
@astrogeco
Copy link
Contributor

astrogeco commented Feb 5, 2021

One reason the original templates had

on: 
  push: 
    - main  
  pull_request:

is that for the integration-candidate PRs everything was running twice. I don't think that's a big deal for thei workflow though so I'm fine keeping it as

on:
  push:
  pull_request:

The other idea is to exclude the integration-candidate from the push trigger but that seems overly complicated

@skliper
Copy link
Contributor

skliper commented Feb 7, 2021

Actually, I wonder if that warning was just coming up because an initial run on main wasn't recorded yet... I bet the first option might actually be OK. Note I tried both on my CodeQL addition to ci_lab and they both gave me the warning (I think because it hasn't been run on main yet).

@ArielSAdamsNASA
Copy link
Contributor Author

ArielSAdamsNASA commented Feb 8, 2021

Actually, I wonder if that warning was just coming up because an initial run on main wasn't recorded yet... I bet the first option might actually be OK. Note I tried both on my CodeQL addition to ci_lab and they both gave me the warning (I think because it hasn't been run on main yet).

The first option being the workflow only using the main branch on push?

I do not see the warning when the main branch is ran on both push and pull-request which is how it is set up currently. However, the error was still occurring multiple times before the main branch was set on both push and pull-request (only set on push originally). I do still want to make some minor changes to the CodeQL workflow that resembles @skliper's ci_lab workflow which includes:

  • Setting timeout
  • Removing the run cFS stage
  • Removing the configuration file and adding the queries to the workflow directly

Latest ci_lab CodeQL run (main branch on push and pull-request):
image

Latest cFS CodeQL run (main branch on push and pull-request):
image

@astrogeco
Copy link
Contributor

What is the downside of having the separate configuration file? It might be useful in case we wanted to use CodeQL locally: https://codeql.github.com/docs/codeql-cli/

@ArielSAdamsNASA
Copy link
Contributor Author

ArielSAdamsNASA commented Feb 8, 2021

What is the downside of having the separate configuration file? It might be useful in case we wanted to use CodeQL locally: https://codeql.github.com/docs/codeql-cli/

@astrogeco There's not really a downside of having a separate configuration file besides the project having an extra folder and file. I can keep the configuration file. So, the ci_lab repo should also use a configuration file for its CodeQL workflow?

@skliper
Copy link
Contributor

skliper commented Feb 9, 2021

@astrogeco from what I've seen from the CLI, everything we need can be specified on the command line. The only thing that was in the configuration file was the queries (just the default two), which can be specified in the workflow or on the command line without maintaining an extra file. I see it as just another level of indirection that doesn't really provide much benefit. That said, it's trivial to change either way if someone wants to add CodeQL CLI makefile rules or whatever so I'd suggest deferring decisions such as this to when it actually gets implemented. Things may change between now and then, so excessive thrash at this point for something that may or may not happen doesn't seem to provide much value. If we start adding custom queries to cover our coding standards it may be valuable to use a standard config file, but that's in the same boat as future work.

@ArielSAdamsNASA
Copy link
Contributor Author

FYI, I noticed that if you add repository: nasa/cFS in the checkout bundle, you can just put the main branch on push without any warnings or errors.

@ArielSAdamsNASA ArielSAdamsNASA removed the CCB:Ignore Pull Request is NOT ready for discussion. Has open actions. Will be re-examined at by next CCB. label Feb 12, 2021
astrogeco added a commit that referenced this issue Feb 16, 2021
Fix #185, Apply CodeQL to Every Branch
chillfig pushed a commit to chillfig/cFS that referenced this issue Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants