-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
editor/code: Break down CI steps to know what is failing easily #15281
Conversation
6576c40
to
f6b359b
Compare
@lnicola How do you think about this? I did not do same thing for Rust's CI job because their step has a dependency that affects to CI job performance. I would not like to touch them in this PR to avoid to make this PR more complex. |
To be honest, I'm not a fan of this. It's a bunch more complex, and I'm not sure it's worth it over a good comment or message printed in the workflow. |
Yeah, that's closer to what I meant. |
f6b359b
to
19f6f50
Compare
Thanks. I updated my changeset. |
@tetsuharuohzeki thanks, did you see my comments on 19f6f50? |
Sorry, I missed it.
I think this alias (mom run type check) is useful when upgrading TypeScript to fix an error said by TypeScript compiler as just like |
19f6f50
to
6a97fc6
Compare
By this commit 6a97fc6 I break down CI step more |
- run: npm run lint | ||
working-directory: ./editors/code | ||
if: needs.changes.outputs.typescript == 'true' | ||
|
||
# To fix this steps, please run `npm run format`. |
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.
Why is this twice? Once here...
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.
Ugh, that was my mistake. I pushed new patch.
.github/workflows/ci.yaml
Outdated
- name: Run VS Code tests (Linux) | ||
if: matrix.os == 'ubuntu-latest' && needs.changes.outputs.typescript == 'true' | ||
env: | ||
VSCODE_CLI: 1 | ||
run: xvfb-run npm test | ||
working-directory: ./editors/code | ||
|
||
# To fix this steps, please run `npm run format`. |
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.
once here.
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 same reason with above
To do this change, we reorganize npm-script. | previous | after | |--------------------|----------------------------------------| | `npm run lint` | `npm run lint && npm run format:check` | | `npm run fix` | `npm run lint:fix && npm run format` | The previous `npm run fix` sometimes does not complete fix automatically because ESLint's autofix doees not follow prettier's formatting. So we need to run `npm run lint:fix && npm run format` by this order.
6a97fc6
to
5cca093
Compare
@bors r+ |
☀️ Test successful - checks-actions |
@lnicola Thank you for your review! |
This do the thing I mentioned in #15265 (comment)
This aims to improve CI status check more readable.
I tried to use
jobs.<job_id>.if
to make the configurationmore shortly once.
But it could not fire the
end-success
orend-failure
status if some jobs in the workflow were skipped. This causes an integration problem with bors.By their reasons, this patch still uses
jobs.<job_id>.steps[*].if
.To do this change, we reorganize npm-script.
npm run lint
npm run lint && npm run format:check
npm run fix
npm run lint:fix && npm run format
The previous
npm run fix
sometimes does not complete fix automatically because ESLint's autofix doees not follow prettier's formatting. So we need to runnpm run lint:fix && npm run format
by this order.