-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Conversation
Netlify deploy previewhttps://deploy-preview-43066--material-ui.netlify.app/ Bundle size report |
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 had a quick dive into the security of the NPM_TOKEN
token.
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).
on: | ||
push: | ||
branches: | ||
- master | ||
- next |
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.
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:
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:
- (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.
- 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.
- 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.
- Option C.
- Option B.
For external dependencies:
- 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.
- Option C.
- Option B.
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 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.
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.
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.
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.
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.
I changed the action to be only triggered manually, as we agreed on. |
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 become6.0.0-alpha.0-dev...
, not6.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...