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

Feature flag to disable python dependency installation #1676

Merged
merged 10 commits into from
May 16, 2023

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented May 9, 2023

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 have tested this behavior on a repo of my own, by setting CODEQL_ACTION_DISABLE_PYTHON_DEPENDENCY_INSTALLATION: "true" in the workflow -- let me know if we need to add some automated tests for this 👍

src/analyze.ts Fixed Show resolved Hide resolved
@RasmusWL RasmusWL force-pushed the rasmuswl/python-disable-dependency-installation branch 6 times, most recently from 18b5112 to 901968a Compare May 11, 2023 10:02
@RasmusWL RasmusWL force-pushed the rasmuswl/python-disable-dependency-installation branch from 901968a to 0ccdbf8 Compare May 11, 2023 10:14
@RasmusWL RasmusWL marked this pull request as ready for review May 11, 2023 10:22
@RasmusWL RasmusWL requested a review from a team as a code owner May 11, 2023 10:22
@RasmusWL RasmusWL requested review from tausbn and turbo May 11, 2023 10:23
@RasmusWL
Copy link
Member Author

@turbo requesting your review on the CHANGELOG.md change

@tausbn requesting your review on the recommendation if users want to specify what version to analyze as

src/analyze.ts Outdated Show resolved Hide resolved
@aeisenberg
Copy link
Contributor

aeisenberg commented May 11, 2023

Also, do you have a related feature flag issue for this change? https://github.com/github/codeql-core/issues/new?assignees=&labels=CodeQL+Action&projects=&template=codeql_action_feature_flag_rollout.md&title=CodeQL+Action%3A+%5BFeature+Name%5D+Rollout+Plan

It helps us manage the rollout and eventual removal of the flag.

@RasmusWL
Copy link
Member Author

Also, do you have a related feature flag issue for this change?

yes, it might be hard to spot from all the highlights of me force pushing changes, but here it is: https://github.com/github/codeql-core/issues/3552

@RasmusWL RasmusWL requested a review from aeisenberg May 11, 2023 17:50
@aeisenberg
Copy link
Contributor

Thanks for the explanation (and apologies for jumping in with partial information).

RasmusWL added 5 commits May 12, 2023 09:55
The last dot in `=3.11.` is just slightly confusing, so added single
quotes around the environment variable assignments to make it 100% clear
tausbn
tausbn previously approved these changes May 12, 2023
Copy link

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

aeisenberg
aeisenberg previously approved these changes May 12, 2023
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 and a suggestion for potentially improving one of the warnings.

src/feature-flags.ts Show resolved Hide resolved
src/analyze.ts Outdated Show resolved Hide resolved
Co-authored-by: Henry Mercer <henry.mercer@me.com>
@RasmusWL RasmusWL requested a review from henrymercer May 15, 2023 10:06
henrymercer
henrymercer previously approved these changes May 15, 2023
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.

I had a quick look at the changenote — I'm happy with the changes here, but also happy to wait for @turbo's review.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Henry Mercer <henry.mercer@me.com>
@@ -2,6 +2,7 @@

## [UNRELEASED]

- We are rolling out a feature in May 2023 that will disable Python dependency installation for new users of the CodeQL Action. This improves the speed of analysis while having only a very minor impact on results. [#1676](https://github.com/github/codeql-action/pull/1676)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we planning on doing a changelog post or some other public announcement? Especially explaining "a very minor impact on results" would be nice. If there is one, can you add a link here? If the post is planned, but the URL isn't known yet, we can always update this entry.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, once we start roll it out to new users, there will be a public changelog post 👍

@RasmusWL RasmusWL merged commit 5489416 into main May 16, 2023
@RasmusWL RasmusWL deleted the rasmuswl/python-disable-dependency-installation branch May 16, 2023 08:40
@github-actions github-actions bot mentioned this pull request May 24, 2023
6 tasks
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.

4 participants