-
Notifications
You must be signed in to change notification settings - Fork 558
(compat) Updated build-tools to check that layer generation is updated during release #25873
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?
Conversation
|
I think I mentioned this at some point, but the release tool is not used by most folks who do releases, so this change may not be that useful to integrate. If you really want to enforce things you need to have something that automatically breaks the build if someone doesn't do it. How does someone run it manually if they're not using the release tool? How important is it that this step of the release happen at a specific time? For example, could it happen the following day when type tests are updated? Does it need to happen only on main or on the release branches as well? Those details need to go into the release documentation. |
| const result = await execa.command(`pnpm run -r layerGeneration:gen`, { | ||
| cwd: context.root, | ||
| }); | ||
| log.verbose(result.stdout); |
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.
Is there a reason we can't call the code that this command invokes? Exec-ing is the least preferred method of integrating if we can just call a function.
| */ | ||
| export interface IFluidCompatibilityMetadata { | ||
| /** The generation of a Fluid layer which is used for validating layer compatibility */ | ||
| /** The current generation of a Fluid layer which is used for validating layer compatibility */ |
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 current generation of a Fluid layer which is used for validating layer compatibility */ | |
| /** The current compat generation of a Fluid layer which is used for validating layer compatibility */ |
| * @param data - An object with handler-specific contextual data. | ||
| * @returns True if the state was handled; false otherwise. | ||
| */ | ||
| export const checkLayerCompatGeneration: StateHandlerFunction = async ( |
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 terminology still really confuses me here. We have a "layer check" tool. Are these the same layers?
I was planning to add steps to the release documentation to preform these steps for cases where these steps are manually done. How do I enforce that the build breaks if this is not done? Are there examples of other tools that do it? It needs to happen at release before release happens and packages are bumped - similar to assert tagging. I discussed with @scottn12 about doing this after the step but that turns out be complicated and delays when generation is updated. It doesn't need to happen in release branches. Those releases are patch release, correct? Which release documentation do I add it to? |
For more context @tylerbutler, we had initially hoped to include this as part of an automated step after the release branch is cut (at the same time as we bump package versions). However, doing it after the release added complications that don't exist if we execute it prior to the release. Doing it before the release introduces other risks - mainly that it would need to be run manually. This means release engineers can forget to run the command or make some type of mistake. As Navin mentioned, we plan to update the release instructions to include this additional step, and I was planning to make an announcment to the release engineers to notify them about it. This PR mainly serves to provide an additional check/warning if release engineers use the release tool command. I know that most release engineers currently don't, but it seems likely that the release rotations may change soon and new release engineers may start out using the tool (it is included in the release instructions). |
|
Maybe it would make sense (not in this PR) to combine our release prep steps in some way. For example, we could combine assert tagging with layer generation updating (Navin had suggested something similar). It may be worth considering updating or adding some type of tool, especially if our release prep steps continue to expand. I think it makes sense to introduce this command separately for now though. |
| * Licensed under the MIT License. | ||
| * | ||
| * THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | ||
| * THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY |
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.
What this done intentionally? I don't recall seeing a corresponding change to the command that generates this?
|
|
||
| /** | ||
| * Checks that a release branch exists. | ||
| * Checks whether assert tagging has been done. |
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.
Good catch!
Added a new check function that will run during a release. For minor and patch releases, it will check the generation for layer compat has been updated before releasing and bumping versions. The check function runs the
pnpm run -r layerGeneration:gencommand and if there are any changes, prompts to commit them before proceeding with the release. This is similar to how type tests and release notes are updated.Follow up to #25670 and #25835.
AB#51927