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

Delete python dependency installation code #2224

Merged
merged 21 commits into from
Apr 9, 2024

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Apr 4, 2024

Draft since we need to wait for 2.17.0 to be fully rolled out first.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

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)
@RasmusWL RasmusWL marked this pull request as ready for review April 8, 2024 08:48
@RasmusWL RasmusWL requested review from a team as code owners April 8, 2024 08:48
@henrymercer henrymercer added the Rebuild Re-transpile JS & re-generate workflows label Apr 8, 2024
@github-actions github-actions bot removed the Rebuild Re-transpile JS & re-generate workflows label Apr 8, 2024
Copy link
Contributor

github-actions bot commented Apr 8, 2024

Pushed a commit to rebuild the Action. Please mark the PR as ready for review to trigger PR checks.

@github-actions github-actions bot marked this pull request as draft April 8, 2024 09:27
@henrymercer henrymercer marked this pull request as ready for review April 8, 2024 09:27
Copy link
Contributor

@henrymercer henrymercer left a 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).

CHANGELOG.md Outdated Show resolved Hide resolved
features: FeatureEnablement,
codeql: CodeQL,
) {
async function setupPythonExtractor(logger: Logger) {
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Done in d33e751

src/init-action.ts Outdated Show resolved Hide resolved
henrymercer
henrymercer previously approved these changes Apr 9, 2024
Copy link
Contributor

@henrymercer henrymercer left a 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.

init/action.yml Outdated Show resolved Hide resolved
src/analyze.ts Outdated Show resolved Hide resolved
} 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")) {
Copy link
Contributor

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?

Copy link
Member Author

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.

@henrymercer henrymercer added the Rebuild Re-transpile JS & re-generate workflows label Apr 9, 2024
@github-actions github-actions bot removed the Rebuild Re-transpile JS & re-generate workflows label Apr 9, 2024
Copy link
Contributor

github-actions bot commented Apr 9, 2024

Pushed a commit to rebuild the Action. Please mark the PR as ready for review to trigger PR checks.

@github-actions github-actions bot marked this pull request as draft April 9, 2024 10:47
@henrymercer henrymercer marked this pull request as ready for review April 9, 2024 10:47
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Thanks again! 🧹

@RasmusWL RasmusWL merged commit 21eac7c into main Apr 9, 2024
297 checks passed
@RasmusWL RasmusWL deleted the RasmusWL/remove-python-dep-inst branch April 9, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants