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] Run react-17 and react-next workflows on the next branch #42690

Merged
merged 16 commits into from
Jul 22, 2024

Conversation

cherniavskii
Copy link
Member

Currently, the react-17 and react-next workflows are scheduled to run every day on the master branch.
Since next is now an active development branch, they should also be run on the next branch.

The react-next workflow on the master branch fails: https://app.circleci.com/pipelines/github/mui/material-ui/131952
It cannot install dependencies, so I added the --frozen-lockfile flag to the pnpm install command.

Finally, both react-17 and react-next workflows can be run on demand using the CircleCI "Trigger pipeline".

@cherniavskii cherniavskii added the scope: code-infra Specific to the core-infra product label Jun 19, 2024
@mui-bot
Copy link

mui-bot commented Jun 19, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 769c02c

pnpm install
else
echo "pnpm install --no-frozen-lockfile"
pnpm install --no-frozen-lockfile
Copy link
Member Author

Choose a reason for hiding this comment

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

The useReactVersion script modifies resolutions in package.json, and it can't install dependencies without this flag.
If there were no changes done to package.json (git --no-pager diff HEAD) == "" ), the flag is not being passed.

Copy link
Member

@Janpot Janpot Jul 15, 2024

Choose a reason for hiding this comment

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

Would it make sense for the useReactVersion script to run pnpm install after it sets the resolutions? That would make the lockfile consistent with what's installed and this branching becomes unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, running useReactVersion without installing the dependencies does not make much sense anyway 👍🏻

react-17:
when:
equal: [react-17, << pipeline.parameters.workflow >>]
jobs:
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to duplicate the jobs here, I couldn't find a way to reuse them in CircleCI (the experience of configuring Circle CI is not great TBH 😅).

The whole point is that:
react-17 workflow can be triggered manually on any branch (e.g. in a PR) on demand through Circle CI.
react-17-cron workflow is triggered by a cron job (same as before this PR).
Same for react-next workflows.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's not great. Perhaps an orb could fix duplication here, but I don't really mind it so much.

@@ -937,6 +988,7 @@ workflows:
branches:
only:
- master
- next
Copy link
Member Author

Choose a reason for hiding this comment

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

Added next branch here for the typescript-next workflow as well.

- test_unit:
<<: *default-context
react-version: next
name: test_unit-react@next
Copy link
Member Author

Choose a reason for hiding this comment

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

Added names here to avoid confusing job names, like this:

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why these ghost jobs appear though:

Perhaps it's a lag on the CircleCI side.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it got in a bad state after renaming those jobs? Maybe try opening a different PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried in #42991 and the result is the same...

Copy link
Member

@Janpot Janpot Jul 22, 2024

Choose a reason for hiding this comment

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

I believe circleci reuses some existing state when opening a PR for the same commit. At least I think I've observed that before. Trying out that hypothesis in mui/mui-x#13942

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that seems to work!
I'll trigger the react-next workflow to make sure everything works as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

All looks good now!
Any objections to merging current PR?

Copy link
Member

Choose a reason for hiding this comment

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

No, go for it 👍

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli Jul 23, 2024

Choose a reason for hiding this comment

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

Seeing these empty CI jobs after merging this in #42990 and other latest PRs. What can I do?

@cherniavskii cherniavskii requested a review from a team July 8, 2024 21:02
@cherniavskii cherniavskii marked this pull request as ready for review July 9, 2024 07:52
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.

Seems good, I'd consider running pnpm install inside the script to keep the lockfile consistent, and to avoid leaking the change into other parts of the CI. But I'm not quite sure if that will have any undesirable side-effects or not.

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.

Approving though as my comment is non-blocking. Feel free to look into it as part of this PR, or later, or not at all 🙂 .

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2024
@cherniavskii cherniavskii merged commit 288863b into mui:next Jul 22, 2024
19 checks passed
@cherniavskii cherniavskii deleted the run-react-workflows-on-next branch July 22, 2024 14:27
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jul 25, 2024
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
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