-
Notifications
You must be signed in to change notification settings - Fork 557
(compat) Add logic to auto update layer compat generation during release #25670
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
*/ | ||
|
||
import { strict as assert } from "node:assert"; | ||
import { readFile, writeFile } from "node:fs/promises"; | ||
import type { Machine } from "jssm"; | ||
import chalk from "picocolors"; | ||
|
||
|
@@ -191,3 +192,109 @@ export const doReleaseGroupBump: StateHandlerFunction = async ( | |
BaseStateHandler.signalSuccess(machine, state); | ||
return true; | ||
}; | ||
|
||
/** | ||
* Updates the generation used for layer compatibility. | ||
* | ||
* @param state - The current state machine state. | ||
* @param machine - The state machine. | ||
* @param testMode - Set to true to run function in test mode. | ||
* @param log - A logger that the function can use for logging. | ||
* @param data - An object with handler-specific contextual data. | ||
* @returns True if the state was handled; false otherwise. | ||
*/ | ||
export const doLayerGenerationUpdate: StateHandlerFunction = async ( | ||
state: MachineState, | ||
machine: Machine<unknown>, | ||
testMode: boolean, | ||
log: CommandLogger, | ||
data: FluidReleaseStateHandlerData, | ||
): Promise<boolean> => { | ||
if (testMode) return true; | ||
|
||
const { bumpType } = data; | ||
if (bumpType === "patch") { | ||
log.info(`Generation update is not needed for patch release`); | ||
BaseStateHandler.signalSuccess(machine, state); | ||
return true; | ||
} | ||
|
||
log.info(`Updating layerGeneration.ts for a ${bumpType} release`); | ||
|
||
// eslint-disable-next-line no-warning-comments | ||
// TODO: Is it okay to read a file from this folder? | ||
// The file that stores information of the current generation and release date. | ||
const filename = "packages/common/client-utils/src/layerGeneration.ts"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calrify that this is relative to the repo root. |
||
|
||
// eslint-disable-next-line no-warning-comments | ||
// TODO: This should ideally be read from a common location in client utils. What is the best way to do this? | ||
// The minimum compatibility window in months that is supported across all layers. | ||
const minimumCompatWindowMonths = 3; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The minimum compatibility window is hardcoded as a magic number. Consider defining this as a named constant at the module level or reading it from a configuration file to improve maintainability. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can read config settings from the fluidBuild config. Maybe it makes sense to store this config data there? |
||
|
||
const today = new Date(); | ||
let previousGeneration = -1; | ||
let newGeneration = 1; | ||
|
||
let fileContents: string | undefined; | ||
try { | ||
fileContents = await readFile(filename, "utf8"); | ||
} catch { | ||
log.info(`File ${filename} doesn't exist yet`); | ||
} | ||
|
||
if (fileContents !== undefined) { | ||
const match = fileContents.match( | ||
/.*\nexport const generation = (\d+);[\n\r]*export const releaseDate = "((0[1-9]|1[0-2])\/(0[1-9]|1\d|2\d|3[01])\/(19|20)\d{2})";.*/m, | ||
); | ||
Comment on lines
+246
to
+248
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The regex pattern is complex and fragile. Consider parsing the TypeScript file using a proper parser or extracting the values using a more robust approach that doesn't rely on exact formatting. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we control the packages, consider putting the date and time of release in the package itself then we can just read it from the previous version. |
||
if (match === null) { | ||
log.errorLog(`${filename} content not as expected`); | ||
BaseStateHandler.signalFailure(machine, state); | ||
return false; | ||
} | ||
|
||
previousGeneration = Number(match[1]); | ||
const previousReleaseDateString = match[2]; | ||
const previousReleaseDate = new Date(previousReleaseDateString); | ||
const daysBetweenReleases = Math.round( | ||
(today.getTime() - previousReleaseDate.getTime()) / (1000 * 60 * 60 * 24), | ||
); | ||
|
||
const monthsBetweenReleases = Math.floor(daysBetweenReleases / 30); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a fixed 30-day month approximation is inaccurate for month calculations. Consider using date arithmetic that accounts for actual month boundaries, such as calculating the difference in months and years between the two dates. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just use 100 days for 3 months, 200 for six months, and 400 for 12 months, etc? |
||
newGeneration = | ||
previousGeneration + Math.min(monthsBetweenReleases, minimumCompatWindowMonths - 1); | ||
|
||
log.info(`Previous release date: ${previousReleaseDate}, Today: ${today}`); | ||
log.info(`Days between releases: ${daysBetweenReleases}`); | ||
log.info(`Months between releases: ${monthsBetweenReleases}`); | ||
} | ||
|
||
if (newGeneration === previousGeneration) { | ||
log.info(`Generation does not need to be bumped`); | ||
BaseStateHandler.signalSuccess(machine, state); | ||
return true; | ||
} | ||
|
||
const yyyy = today.getFullYear(); | ||
const mmNumber = today.getMonth() + 1; // Months start at 0! | ||
const ddNumber = today.getDate(); | ||
|
||
const dd = ddNumber < 10 ? `0${ddNumber}` : ddNumber.toString(); | ||
const mm = mmNumber < 10 ? `0${mmNumber}` : mmNumber.toString(); | ||
const releaseDateFormatted = `${mm}/${dd}/${yyyy}`; | ||
|
||
const output = `/*! | ||
* Copyright (c) Microsoft Corporation and contributors. All rights reserved. | ||
* Licensed under the MIT License. | ||
* THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY | ||
*/ | ||
|
||
export const generation = ${newGeneration}; | ||
export const releaseDate = "${releaseDateFormatted}"; | ||
`; | ||
|
||
log.info(`Bumping generation from ${previousGeneration} to ${newGeneration}`); | ||
await writeFile(filename, output); | ||
|
||
BaseStateHandler.signalSuccess(machine, state); | ||
return true; | ||
}; |
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.
The hardcoded file path makes the code brittle to directory structure changes. Consider using path resolution utilities or making this configurable.
Copilot uses AI. Check for mistakes.