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

Create codeql.yml #4705

Merged
merged 3 commits into from Oct 25, 2022
Merged

Create codeql.yml #4705

merged 3 commits into from Oct 25, 2022

Conversation

trws
Copy link
Member

@trws trws commented Oct 20, 2022

Initial attempt at configuring building and setup for the github version of LGTM scanning. Moving to my own fork.

@trws trws force-pushed the github-code-scanning branch 4 times, most recently from d975d1c to b0b7150 Compare October 20, 2022 16:41
@trws trws requested review from vsoch and grondo October 20, 2022 16:42
@@ -0,0 +1,36 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

This is much neater! Can we do bash strict mode so a failure will kill the build?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh absolutely, I just wasn't thinking about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I'll push it up once we get some results back from the currently enqueued CodeQL run so I can see if anything is still breaking. Hitting some delays on getting the jobs picked up.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! My experience with codeQL (I've just used for Python) is that it's fairly slow. I have it on a few scattered projects and it has never hit any issues so I'm undecided about if I like it / if it's worth it - if it's not any slower than the CI here it shouldn't add time, but if it extends the running time we might want to consider just the scheduled run, or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be, I'm hoping it will be an adequate replacement for LGTM, since it's literally the same code query system by the same team under the hood.

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

Great! Thanks for taking care of this. I just found one thing I had a question on, otherwise it will be interesting to see how this action "comments" on PRs vs LGTM.
(Since LGTM is going away, though, I suppose that is neither here nor there..)

.github/workflows/codeql.yml Outdated Show resolved Hide resolved
@grondo
Copy link
Contributor

grondo commented Oct 20, 2022

This PR could also probably remove .lgtm.yml.

@vsoch
Copy link
Member

vsoch commented Oct 20, 2022

Thank you for your service, LGTM! 🖖

@grondo
Copy link
Contributor

grondo commented Oct 20, 2022

Thank you for your service, LGTM!

Oh, I see what you did there!

@vsoch
Copy link
Member

vsoch commented Oct 20, 2022

@trws the errors are because, depending on whether you are building the container vs. in a container, the two different cases will differ in requiring sudo (required in the run here) or not (a traditional container build). So likely we want an envar that comes from the action that controls that.

This is where I saw this before: https://github.com/rse-ops/actions-cleaner/blob/0ab29f4f4047bb0809a31b5922ab7d04f40d0ab2/ubuntu/action.yml#L131-L136

@grondo
Copy link
Contributor

grondo commented Oct 20, 2022

So likely we want an envar that comes from the action that controls that.

Could you just do

    - name: Install ubuntu dependencies
      run: sudo ./etc/docker/ubuntu/install_deps.sh

@vsoch
Copy link
Member

vsoch commented Oct 20, 2022

Yes! Likely that will be the fix since this particular action will always require it. The case I linked needed the dynamic-ness because it could be run in either context as an action (e.g., using it within a user's container or outside of directly on the runner).

@trws trws force-pushed the github-code-scanning branch 4 times, most recently from 6fb4976 to 5982672 Compare October 21, 2022 18:37
@trws trws marked this pull request as ready for review October 21, 2022 18:37
@trws
Copy link
Member Author

trws commented Oct 21, 2022

Ok, rebases done. Assuming I didn't break anything, this should be ready for a clean review.

@trws
Copy link
Member Author

trws commented Oct 21, 2022

I removed the LGTM config file, but the result is not what I expected. It's still getting run, just without configuration. Seems like we'll have to deal with that manually and turn off the integration.

Initial attempt at configuring building and setup for the github version of LGTM scanning.
@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #4705 (5318362) into master (a45fe69) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 5318362 differs from pull request most recent head 75731ff. Consider uploading reports for the commit 75731ff to get more accurate results

@@            Coverage Diff             @@
##           master    #4705      +/-   ##
==========================================
+ Coverage   83.35%   83.38%   +0.02%     
==========================================
  Files         413      413              
  Lines       69664    69489     -175     
==========================================
- Hits        58071    57941     -130     
+ Misses      11593    11548      -45     
Impacted Files Coverage Δ
src/common/libsubprocess/subprocess.c 87.89% <0.00%> (-0.30%) ⬇️
src/modules/kvs-watch/kvs-watch.c 81.22% <0.00%> (-0.20%) ⬇️
src/cmd/flux-jobs.py 95.52% <0.00%> (ø)
src/broker/boot_config.c 81.74% <0.00%> (+0.02%) ⬆️
src/bindings/lua/flux-lua.c 87.71% <0.00%> (+0.03%) ⬆️
src/connectors/loop/loop.c 82.25% <0.00%> (+0.29%) ⬆️
src/modules/job-info/guest_watch.c 76.75% <0.00%> (+0.54%) ⬆️
src/broker/boot_pmi.c 67.61% <0.00%> (+0.67%) ⬆️
src/broker/topology.c 93.46% <0.00%> (+0.76%) ⬆️
src/broker/overlay.c 86.48% <0.00%> (+0.90%) ⬆️
... and 3 more

@mergify mergify bot merged commit ad1193a into flux-framework:master Oct 25, 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 this pull request may close these issues.

None yet

3 participants