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

Python: Handle new default for standard library extraction #2536

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

yoff
Copy link
Contributor

@yoff yoff commented Oct 9, 2024

When you review this, could you also confirm my thinking on the following?

  • With the change to feature-flags.ts:
    • Feature.CodeqlActionPythonDefaultIsToNotExtractStdlib will be(come) true iff
      (((the feature flag is on AND NOT the disabling feature flag is on) OR the environment variable is set to true) AND NOT the environment variable is set to false) AND ToolsFeature.PythonDefaultIsToNotExtractStdlib is true.
    • That is, this one entry is enough to make the action react to both the feature flag, the disabling feature flag (x_disabled), and the environment variable. And the tools feature takes precedence, then the environment variable, then the disabling feature flag, then the feature flag.
    • The environment variable "CODEQL_ACTION_DISABLE_PYTHON_STANDARD_LIBRARY_EXTRACTION" is now available to the action.
    • People can write something in their workflow file to turn off the feature.
  • With the changes to src/tools-features.ts:
    • ToolsFeature.PythonDefaultIsToNotExtractStdlib is now received from the CLI as long as the entry in there is called
      pythonDefaultIsToNotExtractStdlib
    • if the CLI is not new enough to define this feature, ToolsFeature.PythonDefaultIsToNotExtractStdlib will simply be off.

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.

src/init-action.ts Fixed Show fixed Hide fixed
src/init-action.ts Fixed Show fixed Hide fixed
src/init-action.ts Fixed Show fixed Hide fixed
src/init-action.ts Fixed Show fixed Hide fixed
src/init-action.ts Fixed Show fixed Hide fixed
probably still need to route some values around
@yoff yoff force-pushed the python/ff-std-lib-extraction branch from fcaf886 to ce5f900 Compare October 9, 2024 15:14
@yoff yoff added the Rebuild Re-transpile JS & re-generate workflows label Oct 9, 2024
@yoff yoff marked this pull request as ready for review October 9, 2024 20:50
@yoff yoff requested a review from a team as a code owner October 9, 2024 20:50
@yoff yoff assigned yoff and tausbn and unassigned yoff Oct 9, 2024
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! 👍
(However, I'm not familiar with most aspects of how codeql-action works, so we should probably get someone from the core team to give the go-ahead before merging.)

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.

LGTM, both on the thinking and the code :)

@yoff yoff merged commit 0c3e006 into github:main Oct 11, 2024
540 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rebuild Re-transpile JS & re-generate workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants