-
Notifications
You must be signed in to change notification settings - Fork 334
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
Cache feature flags on disk #1384
Conversation
This will allow feature flags to be shared across steps in the same job, avoiding an error we saw earlier where the init action had the flag enabled, but the analyze step had it disabled. This uses the runner's temp folder to cache the flags file, which will stick around until the job completes.
fs.writeFileSync( | ||
cachedFeatureFlags, | ||
JSON.stringify(actualFeatureEnablement) | ||
); |
Check failure
Code scanning / CodeQL
Potential file system race condition
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.
Huh.. should we be checking files we write to?
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.
This does feel like an FP. I will contact the team.
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!! Makes sense, just a couple of naming thoughts
fs.writeFileSync( | ||
cachedFeatureFlags, | ||
JSON.stringify(actualFeatureEnablement) | ||
); |
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.
Huh.. should we be checking files we write to?
src/feature-flags.ts
Outdated
@@ -55,6 +58,8 @@ export const featureConfig: Record< | |||
*/ | |||
type GitHubFeatureFlagsApiResponse = Partial<Record<Feature, boolean>>; | |||
|
|||
export const FEATURE_FLAGS_FILE_NAME = "feature-flags.json"; |
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.
Wondering if this could be renamed to include local
or cache
somehow so it's easier to remember the difference between this file and the remote server-side flags 🤔
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! Extra test looks 👍
This will allow feature flags to be shared across steps in the same job, avoiding an error we saw earlier where the init action had the flag enabled, but the analyze step had it disabled.
This uses the runner's temp folder to cache the flags file, which will stick around until the job completes.
Merge / deployment checklist