Skip to content
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

Merged
merged 27 commits into from
May 8, 2024

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Apr 17, 2024

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:

  1. Check if there are any uncommitted changes. Fail if there are.
  2. Set up .npmrc to look for an access token in an enviroment variable.
  3. Figure out which packages changed between this and the previous commit.
  4. Update package.json of the changed packages, adding the -dev.[yyMMddHHmmss] suffix.
  5. Publish the changed packages.
  6. Restore package.json files to the original state.

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.

@michaldudak michaldudak added the scope: code-infra Specific to the core-infra product label Apr 17, 2024
@michaldudak michaldudak requested a review from a team April 17, 2024 21:01
@mui-bot
Copy link

mui-bot commented Apr 17, 2024

Netlify deploy preview

https://deploy-preview-41949--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against c7d7190

@Janpot
Copy link
Member

Janpot commented Apr 18, 2024

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
start situation: A@v1=>B@v1 + B@v1
commit 1: only A changes => publish only A, new situation: A@v2=>B@v1 + B@v1, ✅
commit 2: only B changes => publish only B, new situation: A@v1=>B@v1 + B@v2, ❌
=> don't have a version of A that points to B@v2

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, ❌
=> doesn't know about B@v2 because repo always resets versions after publishing

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, ✅
=> now B@v2 is incorporated in 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

@michaldudak
Copy link
Member Author

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.

Only correct way is to always publish everything, changes or not

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).
How about we check for changes between the last commit and the last stable release instead?

@Janpot
Copy link
Member

Janpot commented Apr 18, 2024

How about we check for changes between the last commit and the last stable release instead?

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 23, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 24, 2024
@michaldudak
Copy link
Member Author

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).

@michaldudak michaldudak marked this pull request as ready for review April 24, 2024 15:28
@michaldudak
Copy link
Member Author

I did a test run - the resulting packages were published to npm with the -dev.240424162023-9968b4889d suffix:

  • @mui/codemod
  • @mui/core-downloads-tracker
  • @mui/joy
  • @mui/material
  • @mui/docs
  • @mui/icons-material
  • @mui/lab
  • @mui/material-nextjs
  • @mui/styles
  • @mui/system


async function setVersion(packages: PackageInfo[]) {
const { stdout: currentRevisionSha } = await $`git rev-parse --short HEAD`;
const timestamp = formatDate(new Date());
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

@Janpot Janpot May 5, 2024

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.

Copy link
Member Author

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}`;
Copy link
Member

@oliviertassinari oliviertassinari May 4, 2024

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

Copy link
Member

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.

Copy link
Member Author

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.

return date
.toISOString()
.replace(/[-:TZ]/g, '')
.slice(2, 14);
Copy link
Member

@oliviertassinari oliviertassinari May 4, 2024

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.

Suggested change
.slice(2, 14);
.slice(0, 14);

Copy link
Member Author

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.

@michaldudak
Copy link
Member Author

Updated the version number to reflect the commit date and changed the format to -dev.yyyyMMdd-hhmmss-sha (for example @mui/material@6.0.0-dev.20240507-080616-4ea4f9e382)

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Good for me 👍

@michaldudak
Copy link
Member Author

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.

@michaldudak michaldudak merged commit 8f88c8b into mui:next May 8, 2024
19 checks passed
@michaldudak michaldudak deleted the publish-script branch May 8, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants