Skip to content

Remove TRAP caching feature flag #1533

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.

Sign up for GitHub

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

Jump to bottom
Merged
cklin merged 1 commit into from
Feb 13, 2023
Merged

Remove TRAP caching feature flag #1533

cklin merged 1 commit into from
Feb 13, 2023

Conversation

@cklin
Copy link
Contributor

@cklin cklin commented Feb 10, 2023 •
edited
Loading

This PR makes the CodeQL action assume that the trap_caching_enabled is true without actually checking it. The feature flag has been set to true globally for months, so this change should have no practical effect.

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.

@cklin cklin requested a review from a team as a code owner February 10, 2023 17:30
angelapwen
angelapwen approved these changes Feb 10, 2023
View reviewed changes
Copy link
Contributor

@angelapwen angelapwen 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, reran the PR check timeout.

henrymercer
henrymercer approved these changes Feb 10, 2023
View reviewed changes
Copy link
Contributor

@henrymercer henrymercer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Typically, removing the feature flag check in the Action would enable the feature on GHES, since features are off by default in GHES. However since getTrapCachingEnabled() returns false for self-hosted runners, TRAP caching is effectively disabled for GHES. So I agree this PR doesn't change behaviour in practice — the only change will be to enable TRAP caching for PRs without permission to access the API, such as Dependabot PRs.

@cklin
Copy link
Contributor Author

cklin commented Feb 10, 2023

Typically, removing the feature flag check in the Action would enable the feature on GHES, since features are off by default in GHES.

Thanks for pointing that out! For future reference, what do we usually do for GHES in feature flag removals? If we want the feature to remain disabled in GHES, we could use util.getGitHubVersion() and specifically exclude GHES. What about features that we do want to enable on GHES?

@henrymercer
Copy link
Contributor

henrymercer commented Feb 10, 2023

Most of the time, we want to enable the feature on GHES, which we do by dropping the feature flag check, and making sure there aren't any code paths to specifically disable GHES, like you mention.

@cklin cklin merged commit e187d07 into github:main Feb 13, 2023
@cklin cklin deleted the trap-caching-feature-flag branch February 13, 2023 14:27
@github-actions github-actions bot mentioned this pull request Feb 24, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henrymercer henrymercer henrymercer approved these changes

+1 more reviewer

@angelapwen angelapwen angelapwen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cklin @henrymercer @angelapwen