-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[code-infra] Add canary release scripts #41949
Conversation
Netlify deploy previewhttps://deploy-preview-41949--material-ui.netlify.app/ Bundle size report |
I'm not 100% sure, but I think there is a problem with only publishing changed packages in this scenario suppose A has a dependency on B And even if you also publish everything with a dependency on B, then you get commit 2: only B changes => publish B and dependants, new situation: A@v2=>B@v2 + B@v2, ✅ but now: commit 3: only A changes => publish only A, new situation: A@v3=>B@v1 + B@v1, ❌ Only correct way is to always publish everything, changes or not commit 3: only A changes => publish everything, new situation: A@v3=>B@v3 + B@v3, ✅ Or am I misunderstanding how this works? If we can't rely on semver resolution then shouldn't the whole dependency tree reflect the desired situation, regardless of changes? I'm also touching on this in https://www.notion.so/mui-org/code-infra-Release-process-299a2fb7fe93441b8e345af0b2fc4fb4?pvs=4#7ba482d1725147bc8f312b6836f64b5e |
I see. The script publishes changed packages and all the packages that depend on it, but "=> doesn't know about B@v2 because repo always resets versions after publishing" is indeed a problem.
This will create a massive number of packages that are essentially the same (especially considering that we don't change the infra code very often). |
That could also work, if that's what we already do for stable releases anyway, I don't see an immediate problem with it. I'm changing https://www.notion.so/mui-org/code-infra-Release-process-299a2fb7fe93441b8e345af0b2fc4fb4?pvs=4#7ba482d1725147bc8f312b6836f64b5e |
I ended up actually implementing both: first, the script checks if there are any changes in public packages between this and the previous commit (there is no point in releasing packages if nothing changed). Then we look for differences between this commit and the latest tag (or any other commit - this is configurable). |
418630e
to
bfbf7d9
Compare
I did a test run - the resulting packages were published to npm with the
|
scripts/canaryRelease.mts
Outdated
|
||
async function setVersion(packages: PackageInfo[]) { | ||
const { stdout: currentRevisionSha } = await $`git rev-parse --short HEAD`; | ||
const timestamp = formatDate(new Date()); |
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.
🤔 I was just thinking, we should be able to read the date from the commit. That way we make it impossible to publish the same commit twice.
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's the issue with publishing the same commit twice?
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.
To clarify, "Twice occasinally" is not really my main concern. It's about making this script idempotent. When multiple runs of the script result in the same outcome. This makes it have an inherent resiliency which comes in handy when we're going to automate it. It becomes safe to do automatic retries, and an accidental misconfiguration can't make you end up with 100 times publishing the same package.
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 point!
newVersion = version.slice(0, dashIndex); | ||
} | ||
|
||
newVersion = `${newVersion}-dev.${timestamp}-${currentRevisionSha}`; |
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.
In the format, is it possible to drop the time from the date? https://www.npmjs.com/package/typescript?activeTab=versions seems clearer and good enough since we have the commit too in our case.
Before:
6.0.0-dev.240424162023-9968b4889d
After:
6.0.0-dev.20240424-9968b4889d
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.
Actually, maybe even?
6.0.0-dev-2024-04-24-9968b4889d
would be clearer.
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 was done to ensure the version suffix is always increasing. In the TypeScript case, it's OK, as they publish one version a day. If we're targeting to publish each commit, having time is required to ensure increasing versions.
scripts/canaryRelease.mts
Outdated
return date | ||
.toISOString() | ||
.replace(/[-:TZ]/g, '') | ||
.slice(2, 14); |
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.
Why slice the year? It feels like it breaks the pattern matching of my brain (and others?) to recognize what the string is about.
.slice(2, 14); | |
.slice(0, 14); |
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.
Does it matter what this is about, though? It's just to ensure that version numbers increase.
Updated the version number to reflect the commit date and changed the format 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.
Good for me 👍
One more addition I made - the ability to ignore certain packages. I thought it could be useful to prevent uploading the huge @mui/icons-material after each commit. |
Created scripts in infra packages that can be used to publish them after each PR to the main (master or next) branch.
The workflow looks like this:
-dev.[yyMMddHHmmss]
suffix.Runnning
pnpm canary:release
executes all these steps.It requires an npm access token present in the NPM_TOKEN environment variable.
I'll create a GitHub action running the script in a separate PR, once this one is merged.