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

ci: use CodeQL instead of LGTM #1186

Merged
merged 1 commit into from
Sep 21, 2022
Merged

ci: use CodeQL instead of LGTM #1186

merged 1 commit into from
Sep 21, 2022

Conversation

mrc0mmand
Copy link
Member

@mrc0mmand mrc0mmand commented Sep 14, 2022

As LGTM is going to be shut down by EOY[0], let's move the code scanning to CodeQL as recommended. Thanks to GH integration the results from such scans will be shown both in the respective PR and in the Security -> Code Scanning tab[1].

[0] https://github.blog/2022-08-15-the-next-step-for-lgtm-com-github-code-scanning/
[1] https://github.com/systemd/mkosi/security/code-scanning


Once this is merged I'll go ahead and disable the LGTM integration completely.

Note: I had to create this branch directly in the upstream repo, otherwise GH wouldn't pick up the new action until it's merged.

@mrc0mmand
Copy link
Member Author

Also a note: the current CodeQL fail is expected, since it's a first scan that found all the possible issues in the code base, which are already reported by LGTM. Also, it's missing the base branch results, which will get resolved once this is merged.

As LGTM is going to be shut down by EOY[0], let's move the code scanning to
CodeQL as recommended. Thanks to GH integration the results from such
scans will be shown both in the respective PR and in the Security ->
Code Scanning tab[1].

[0] https://github.blog/2022-08-15-the-next-step-for-lgtm-com-github-code-scanning/
[1] https://github.com/systemd/mkosi/security/code-scanning
@mrc0mmand mrc0mmand marked this pull request as ready for review September 14, 2022 10:14
@mrc0mmand
Copy link
Member Author

mrc0mmand commented Sep 14, 2022

I enabled the extend query suites, so now it reports quite a number of potential issues, see https://github.com/systemd/mkosi/security/code-scanning?query=pr%3A1186+tool%3ACodeQL+is%3Aopen

If you don't find the extended checks useful, please let me know and I'll disable them again.

@behrmann
Copy link
Contributor

Also a note: the current CodeQL fail is expected, since it's a first scan that found all the possible issues in the code base, which are already reported by LGTM. Also, it's missing the base branch results, which will get resolved once this is merged.

So on the next PR the test will succeed if no new problems are added? Is there a way to see the baseline results afterwards?

@mrc0mmand
Copy link
Member Author

Also a note: the current CodeQL fail is expected, since it's a first scan that found all the possible issues in the code base, which are already reported by LGTM. Also, it's missing the base branch results, which will get resolved once this is merged.

So on the next PR the test will succeed if no new problems are added?

Exactly.

Is there a way to see the baseline results afterwards?

They'll appear in the Security tab under Code Scanning - https://github.com/systemd/mkosi/security/code-scanning

Copy link
Contributor

@behrmann behrmann left a comment

Choose a reason for hiding this comment

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

LGTM. I've scrolled through the warnings and some things are noise, but some do warrant looking into. Since this will stay available I see no reason to not merge this, since it's definitely a leg up from lgtm.com.

One last question though: Does this have a know for which Python version to target or can it read that from e.g. pyproject.toml?

@mrc0mmand
Copy link
Member Author

LGTM. I've scrolled through the warnings and some things are noise, but some do warrant looking into. Since this will stay available I see no reason to not merge this, since it's definitely a leg up from lgtm.com.

One last question though: Does this have a know for which Python version to target or can it read that from e.g. pyproject.toml?

It tries to sort of autodetect stuff from the files lying around:

  Will try to guess Python version, as it was not specified in `lgtm.yml`
  Trying to guess Python version based on Trove classifiers in setup.py
  Found no Trove classifiers for Python in setup.py
  Trying to guess Python version based on travis file
  Did not find any travis files (expected them at either ['/home/runner/work/mkosi/mkosi/.travis.yml', '/home/runner/work/mkosi/mkosi/travis.yml'])
  Trying to guess Python version based on installed versions
  Only Python 3 installed -- will use that version
  Found setup.py, will install package with pip in editable mode

but currently it falls back to the installed python 3.10. However, if the need arises, you can configure the environment as needed before calling the CodeQL analysis.

@mrc0mmand
Copy link
Member Author

Also, to add to this, CodeQL does pip install -e . which should respect the pyproject.toml and other such files (though there's currently a bug in the logic there - github/codeql-action#1249).

@DaanDeMeyer
Copy link
Contributor

I guess we'll need to go over and try to fix the reported warnings before merging this

@mrc0mmand
Copy link
Member Author

I guess we'll need to go over and try to fix the reported warnings before merging this

Well, the warnings won't get reported again (i.e. no noise in other PRs), but stay visible in the Security tab, so they can be fixed later without the need of blocking this (so CodeQL could possibly analyze other PRs).

@DaanDeMeyer DaanDeMeyer merged commit 5f9de8e into main Sep 21, 2022
@mrc0mmand mrc0mmand deleted the codeql branch September 21, 2022 09:48
@poettering
Copy link
Member

So I guess #1120 can be closed now?

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

Successfully merging this pull request may close these issues.

4 participants