[code-infra] Create re-usable code-infra orb #828
Conversation
brijeshb42
added
the
scope: code-infra
Bundle size report
Check out the code infra dashboard for more information about this PR. |
brijeshb42
force-pushed
the
ci-orb
branch
21 times, most recently
from
517b610 to
c8933f5
Compare
brijeshb42
force-pushed
the
ci-orb
branch
4 times, most recently
from
60bcabc to
a5cc973
Compare
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
|
I've moved the orb file at the top level |
.circleci/config.yml
Outdated
| version: 2.1 | ||
|
|
||
| orbs: | ||
| code-infra: https://raw.githubusercontent.com/mui/mui-public/50255f1e368561998ed6dbaf8d78b1a09ff22943/.circleci/orbs/code-infra.yml |
There was a problem hiding this comment.
We'll version this even in the mui/mui-public repo?
There was a problem hiding this comment.
This got left from my testing. It'll be master here.
.circleci/orbs/code-infra.yml
Outdated
| pnpm install | ||
| fi | ||
| run-linters: |
There was a problem hiding this comment.
This makes me question, what with repos where these linters aren't set up yet. e.g. joy for a while was just messing around and didn't care for all of these. Wouldn't make sense to create separate templates for these?
There was a problem hiding this comment.
You mean individual linters? like run-eslint, run-stylelint etc?
If a repo is just starting, they can just not use this command and add lints individually till the point the repo is ready.
There was a problem hiding this comment.
Yeah, or I can imagine some repo not needing things like stylelint or something.
they can just not use this command and add lints individually till the point the repo is ready.
That's interesting, we could also create templates for each of these and use them ourselves in run-linters
There was a problem hiding this comment.
Or we could accept parameters to disable specific linter commands for new repos.
Doesn't make much sense if it's just pnpm eslint or pnpm stylelint. Would have made sense if it was complex it-else or bash, IMO.
There was a problem hiding this comment.
nah, I wouldn't do parameters to enable steps, composability over configurability. I don't think it matters how much is in a step, the criterium should be what level of composability do we want/need, and that's what we should do, even if for some of the steps that's just as simple proxying to pnpm eslint. No worries if you don't see the utility, then let's keep it as is.
.circleci/orbs/code-infra.yml
Outdated
| chmod +x codecov | ||
| ./codecov -t ${CODECOV_TOKEN} -Z -F "<< parameters.key >>" | ||
| check-static-changes: |
There was a problem hiding this comment.
Similar, feels like it could work well as separate tasks too
| # See https://stackoverflow.com/a/73411601 | ||
| command: corepack enable --install-directory ~/bin | ||
| - run: | ||
| name: View install environment |
There was a problem hiding this comment.
Wondering whether these three steps not make more sense in mui-node? Or maybe we need a mui-node-browser as well? 🤔
There was a problem hiding this comment.
But in all the repos, we only specify the executor once. This was done with that in consideration.
We could have two different executors as well and specify it at the job level. It may be cleaner in the job specification.
There was a problem hiding this comment.
But we use two different images right? Would that mean we'd have two different executors?
And should the browser parameter not just follow whichever executor is used? Would it make sense/be possible to create two executors, one for cimg/node and one for the playwright image. Then have a env var in both telling downstream jobs what they are (e.g. MUI_EXECUTOR=node|browser), then use this env var instead of passing the browser parameter to those jobs?
There was a problem hiding this comment.
I saw this mcr.microsoft.com almost every other day in the configs but never saw the docker before it.
I'll add another executor for it. There may be some manual handling required to bump the versions though.
There was a problem hiding this comment.
Ah, it needs to update in lock step with the playwright npm package
brijeshb42
force-pushed
the
ci-orb
branch
4 times, most recently
from
6d126b2 to
8569478
Compare
.circleci/orbs/code-infra.yml
Outdated
| - eslint-cache- | ||
| - run: | ||
| name: ESLint | ||
| command: pnpm exec eslint . --cache --cache-strategy content --report-unused-disable-directives --max-warnings 0 |
There was a problem hiding this comment.
We now don't need to rely on the repo's eslint:ci given it is now independent of the file structure. With one update here, we can run the latest command in all repos.
Same for markdownlint. stylelint still accepts some params that seem to be repo dependent.
There was a problem hiding this comment.
🤔 Not sure about this, from this orb we don't control the version of eslint that's installed in the repo. It could be hard to orchestrate a major upgrade if there's a breaking change in the CI. Also impossible to make local overrides.
edit: oh wait, did you mean pnpm run instead of pnpm exec?
There was a problem hiding this comment.
So to be clear, I'm not against pnpm run eslint --cache --cache-strategy content if you want to remove eslint:ci, I was just thrown off by the usage of pnpm exec. I thought you wanted to recreate what was in the eslint command instead of augment it.
| - run: | ||
| name: Install js dependencies | ||
| command: | | ||
| packages="<< parameters.package-overrides >>" |
There was a problem hiding this comment.
If we have to do so much processing to parse parameters.package-overrides, perhaps we should just do it in the CLI? Keep complexity of the orb low.
oliviertassinari
temporarily deployed
to
ci-orb - mui-tools-public PR #828
Render
Destroyed
|
Added separate playwright executor. Removed the |
Used as a reference here -