-
Notifications
You must be signed in to change notification settings - Fork 335
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
Delete python dependency installation code #2224
Conversation
I've left a few warning logging cases, but overall this feature is no longer supported.
I think we can delete this logic too, but let's deal with that in a separate PR
See github/codeql#16127 (which will be released as part of 2.17.1)
Pushed a commit to rebuild the Action. Please mark the PR as ready for review to trigger PR checks. |
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.
Nice! A couple of comments, I suggest we also bump the version to v3.25.0 for this change (I'm happy to do that in a separate PR if you'd prefer).
features: FeatureEnablement, | ||
codeql: CodeQL, | ||
) { | ||
async function setupPythonExtractor(logger: Logger) { |
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.
Could we print out a warning if the customer is trying to enable dependency installation via CODEQL_ACTION_DISABLE_PYTHON_DEPENDENCY_INSTALLATION
or setup-python-dependencies
stating that these are now ignored and pointing to the changelog blog post / relevant help?
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.
Sure. Done in d33e751
Co-authored-by: Henry Mercer <henrymercer@github.com>
Co-authored-by: Henry Mercer <henrymercer@github.com>
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.
A question around the difference between CodeQL 2.13.1+ and 2.16.0+, and a couple of minor suggestions, but otherwise LGTM.
} else { | ||
// From 2.16.0 the default for the python extractor is to not perform any library | ||
// extraction, so we need to set this flag to enable it. | ||
} else if (await codeQlVersionAbove(codeql, "2.13.1")) { |
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.
Why is this a separate case to 2.16.0? Does the CLI generally expect that dependency extraction is enabled for CodeQL 2.13.1 and later but not 2.16.0 and later?
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.
Yep, spot on!
From 2.16.0 until 2.17.1, if the enrivonment variable is not set, the extractor will print a warning about this (which I wanted to suppress in codeql-action logs).
I guess my thinking was that once we drop support for the last 2.15.x
we could drop one of the else if
branches, but maybe it's OK to just merge them all together.
Co-authored-by: Henry Mercer <henrymercer@github.com>
Pushed a commit to rebuild the Action. Please mark the PR as ready for review to trigger PR checks. |
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.
Thanks again! 🧹
Draft since we need to wait for 2.17.0 to be fully rolled out first.
Merge / deployment checklist