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.

Sign up for GitHub

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

Jump to bottom

ci: add CodeQL analysis #14514

Merged
merged 1 commit into from
Jan 8, 2021
Merged

ci: add CodeQL analysis #14514

merged 1 commit into from
Jan 8, 2021

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Jan 4, 2021

This PR enables CodeQL analysis for the project.

cc @sharlns

@twpayne twpayne added dont-merge/preview-only Only for preview or testing, don't merge it. release-note/ci This PR makes changes to the CI. labels Jan 4, 2021
@twpayne twpayne requested a review from a team as a code owner January 4, 2021 12:37
@twpayne twpayne requested a review from kkourt January 4, 2021 12:37
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 4, 2021
sayboras
sayboras reviewed Jan 4, 2021
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 💯, one quick question on report https://github.com/cilium/cilium/security/code-scanning?query=ref%3Arefs%2Fheads%2Fpr%2Ftwpayne%2Fcodeql-analysis, just curious who will check this report and take any action if required ? 🤔

.github/workflows/codeql-analysis.yml Show resolved Hide resolved
.github/workflows/codeql-analysis.yml Show resolved Hide resolved
@twpayne
Copy link
Contributor Author

twpayne commented Jan 4, 2021

just curious who will check this report and take any action if required ?

Initially me and probably @sharlns 😄 We're both working for Isovalent and have an interest in Cilium's security.

sayboras
sayboras approved these changes Jan 4, 2021
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

aanm
aanm reviewed Jan 6, 2021
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why we don't enable it by default as a blocking requirement to merge PRs? How long does the codeql takes to run? It looked it was less than 3 seconds?

@twpayne
Copy link
Contributor Author

twpayne commented Jan 6, 2021

Any reason why we don't enable it by default as a blocking requirement to merge PRs?

The scanner does occasionally have false positives (e.g. github/codeql-go#439) and not all the problems it identifies are genuine security problems so I think making it a blocking requirement would be too strict.

@twpayne
ci: add CodeQL analysis
6647e70 
Signed-off-by: Tom Payne <tom@isovalent.com>
This was referenced Jan 6, 2021
Merged
Closed
Merged
Merged
kkourt
kkourt approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@twpayne twpayne removed the dont-merge/preview-only Only for preview or testing, don't merge it. label Jan 8, 2021
@twpayne twpayne mentioned this pull request Jan 8, 2021
Merged
@twpayne twpayne added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 8, 2021
@twpayne
Copy link
Contributor Author

twpayne commented Jan 8, 2021

I think this can be merged now. Many of the identified problems have been fixed in other PRs which can be merged one by one.

@pchaigno pchaigno merged commit f0d509e into master Jan 8, 2021
@pchaigno pchaigno deleted the pr/twpayne/codeql-analysis branch January 8, 2021 12:10
@youssefazrak youssefazrak mentioned this pull request Jan 8, 2021
Closed
youssefazrak added a commit to youssefazrak/cilium that referenced this pull request Jan 9, 2021
@youssefazrak
CI: fix cron values for CodeQL analysis
0b73e6c 
On the recent introduction of CodeQL analysis (PR cilium#14514), the cron value has
been entered wrongly.

This PR fix the values of the hours/minutes.

Fixes: cilium#14567

Signed-off-by: Youssef Azrak <yazrak.tech@gmail.com>
twpayne pushed a commit that referenced this pull request Jan 11, 2021
@youssefazrak @twpayne
CI: fix cron values for CodeQL analysis
cf464c4 
On the recent introduction of CodeQL analysis (PR #14514), the cron value has
been entered wrongly.

This PR fix the values of the hours/minutes.

Fixes: #14567

Signed-off-by: Youssef Azrak <yazrak.tech@gmail.com>
pchaigno pushed a commit that referenced this pull request Jan 11, 2021
@youssefazrak @pchaigno
CI: fix cron values for CodeQL analysis
efaa452 
On the recent introduction of CodeQL analysis (PR #14514), the cron value has
been entered wrongly.

This PR fix the values of the hours/minutes.

Fixes: #14567

Signed-off-by: Youssef Azrak <yazrak.tech@gmail.com>
@twpayne twpayne mentioned this pull request Feb 1, 2021
Merged
michi-covalent
michi-covalent reviewed May 20, 2021
View reviewed changes
.github/workflows/codeql-analysis.yml
push:
branches:
- master
- v1.9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

late to the party, but maybe v*.* here? seems pretty easy to forget to update this file after each release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#16251 🚢

also removed the periodic trigger. let me know if we still want to keep it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno left review comments

@aanm aanm aanm left review comments

@michi-covalent michi-covalent michi-covalent left review comments

@kkourt kkourt kkourt approved these changes

@sayboras sayboras sayboras approved these changes

Assignees

@kkourt kkourt

Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@twpayne @kkourt @pchaigno @aanm @sayboras @michi-covalent