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] Automate canary releases #43066

Merged
merged 10 commits into from
Jul 31, 2024

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Jul 25, 2024

Added a GitHub Action to run the canary publish script after each commit to master or next.

Additionally, changed the versioning pattern to include the original prerelease specifier (so 6.0.0-alpha.0 will become 6.0.0-alpha.0-dev..., not 6.0.0-dev... as before).

Unfortunately, I don't see a way to test it in PR, as workflows triggered from PRs don't have access to secrets. I also can't enable manual triggers, as the workflow file needs to be on default branch to be enabled. I tested the access token locally and it publishes without errors. I guess we'll have to test in production...

@michaldudak michaldudak added the scope: code-infra Specific to the core-infra product label Jul 25, 2024
@michaldudak michaldudak requested a review from a team July 25, 2024 08:49
@mui-bot
Copy link

mui-bot commented Jul 25, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against a093fab

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I had a quick dive into the security of the NPM_TOKEN token.

It seems OK, per https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization

SCR-20240726-bmpn

too many people have access to it, but because of cli/cli#3397, it should be fine anyway.


I'm curious, I imagine that the canary releases will have the provenance flag #38321. Edit: ah no, it needs to be provided with --provenance. I guess this could be a follow-up (one problem at a time).

Comment on lines 3 to 7
on:
push:
branches:
- master
- next
Copy link
Member

@oliviertassinari oliviertassinari Jul 25, 2024

Choose a reason for hiding this comment

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

Readding https://www.notion.so/mui-org/code-infra-Release-process-299a2fb7fe93441b8e345af0b2fc4fb4. I'm a bit confused. This diff looks like Option A, but the Shapng page on Noton seems to point toward Option B + C in the proposal, in which case it would be something like this:

Suggested change
on:
push:
branches:
- master
- next
on:
schedule:
- cron: "0 4 * * *"

and the Notion page has Option D in the solution.


My perspective on the options, sorted with the ones that yield the most value first:

For private dependencies:

  1. (the usual 😄) continue to use the git dependency, relying on Add from github source, in a specific subfolder pnpm/pnpm#4765, no npm package intermediate, consume the source straight.
  2. Option D. It seems equivalent to what we have with @mui/monorepo. I haven't seen major pains with code-infra or docs-infra relying on unreleased features of external npm packages.
  3. Option A. While it goes against https://www.w3.org/TR/design-principles/#priority-of-constituencies "User needs come before the needs of web page authors, which come before the needs of user agent implementors, which come before the needs of specification writers, which come before theoretical purity.", between not being able to make a change in docs-infra / code-infra and propagate it in one batch and having end-users struggle with too many versions on our npm public pages; It feels like A overall yields the most value.
  4. Option C.
  5. Option B.

For external dependencies:

  1. Option A. Because for example, I'm the owner of the Slider, so I fix bugs across Base UI and Material UI (like MUI X). I have a bug in Material UI. I need to merge a change in Base UI first, and then go to Material UI to fix the second part. It would be hell if I had to wait.
  2. Option C.
  3. Option B.

Copy link
Member

@Janpot Janpot Jul 26, 2024

Choose a reason for hiding this comment

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

The 1. doesn't work for us because of pnpm/pnpm#7487 (comment) and it doesn't seem like that use-case has high priority.
The problem is that we want a monorepo DX but not use a monorepo. It's a bit like wanting a monolith DX, while using microservices. Perhaps we should look into tooling like bit?

Because for example, I'm the owner of the Slider, so I fix bugs across Base UI and Material UI (like MUI X). I have a bug in Material UI. I need to merge a change in Base UI first, and then go to Material UI to fix the second part. It would be hell if I had to wait.

For this I'd rather look into the direction of linking local dependencies. Building dependencies from source won't scale, it needs to be optional.

Copy link
Member

@oliviertassinari oliviertassinari Jul 26, 2024

Choose a reason for hiding this comment

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

Overall, the main point I wanted to make is that spamming the npm version page, would be nice to avoid but we shouldn't sacrifice our ability to verify quickly propagate changes between repositories. If we have to sacrifice something, why not this. In the past, it might have wrongly given the impression that no bloat in the npm package page was super important, so I wanted to rectify this.

because of pnpm/pnpm#7487 (comment)

Right, but we don't necessarily have to use "workspace" in the dependency 😁. It's not as clear, but not a major cons.

For this I'd rather look into the direction of linking local dependencies. Building dependencies from source won't scale, it needs to be optional.

Right, but linking local dependencies only helps with local development? We still need to merge those changes in the main branch. First in Base UI, and second in Material UI. Actually, sometimes, the fix in Material UI will be broken if it uses the wrong version of Base UI, so we might even need to coordinate this deeper, and wait Base UI release before updating Material UI.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but linking local dependencies only helps with local development?

yes

We still need to merge those changes in the main branch. First in Base UI, and second in Material UI. Actually, sometimes, the fix in Material UI will be broken if it uses the wrong version of Base UI

  • We develop the fix with local linking base.
  • Once we verified the fix works both in base and in core, we can make the PR for base
  • The PR in core can then use the codesandboxci published version of base, if you want to verify CI
  • The fix in core can only be merged after the fix in base has been published.
  • There is no other way in multiple repositories. otherwise you need to coordinate core and base publishing to the second.

@michaldudak
Copy link
Member Author

I changed the action to be only triggered manually, as we agreed on.

@michaldudak michaldudak merged commit 9b04e74 into mui:next Jul 31, 2024
19 checks passed
@michaldudak michaldudak deleted the automate-canary-releases branch July 31, 2024 13:23
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