Improvement/cldsrv 875 integrate shared prettier#6124
Improvement/cldsrv 875 integrate shared prettier#6124benzekrimaha wants to merge 4 commits intodevelopment/9.3from
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review by Claude Code |
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
2 times, most recently
from
e57392c to
e85ae46
Compare
|
|
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
Review by Claude Code |
Review by Claude Code |
|
Review by Claude Code |
|
LGTM |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
@@ Coverage Diff @@
## development/9.3 #6124 +/- ##
===================================================
- Coverage 84.45% 84.36% -0.09%
===================================================
Files 206 206
Lines 13307 13307
===================================================
- Hits 11238 11227 -11
- Misses 2069 2080 +11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
|
|
||
| permissions: | ||
| contents: read | ||
| packages: write |
There was a problem hiding this comment.
The concurrency group uses pull_request.head.sha, which changes with every new commit pushed to the PR. This means each push creates a different concurrency group, so previous runs won't be canceled — defeating the purpose of cancel-in-progress: true.
Use the PR number (or head_ref) instead so that successive pushes to the same PR cancel prior runs:
```suggestion
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }}
|
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
from
16e46ef to
d8df099
Compare
| lint: | ||
| uses: ./.github/workflows/lint.yaml | ||
| with: | ||
| git-diff-args: ${{ github.event_name == 'push' && format('{0}..{1}', github.event.before, github.sha) || 'HEAD~1..HEAD' }} |
There was a problem hiding this comment.
not correct : should run on the diff from "upstream" to now.
for exemple: if we push 1 commit, then another 1 → script must run on the whole diff (2 commits), not just the last one
| else | ||
| yarn run --silent prettier:diff --check | ||
| fi | ||
| MERGE_BASE=$(git merge-base HEAD origin/HEAD) |
There was a problem hiding this comment.
merge-base should work, but not sure about origin/HEAD : may be ok if the latest development branch is indeed the "default" one, but could fail otherwise.
maybe a slightly safer way would be to list all development branches, something like
MERGE_BASE=$(git merge-base HEAD $(git branch --list 'development/*'))
There was a problem hiding this comment.
(to be tested, not sure we have all branches locally or if we need to add the origin/ prefix)
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
from
1a01b41 to
666b8cd
Compare
| pull_request: | ||
| branches-ignore: | ||
| - 'q/*/**' | ||
|
|
There was a problem hiding this comment.
lint.yaml uses actions/checkout@v6 and actions/setup-node@v6 while tests.yaml still uses @v4 for both. This inconsistency could cause subtle differences in checkout behavior (e.g. git config, fetch behavior) between the two workflows. Consider aligning both files on the same major version.
— Claude Code
|
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
from
666b8cd to
eefa22b
Compare
| done | ||
|
|
||
| if [[ "$HAS_MODE" = false ]]; then | ||
| PRETTIER_ARGS=("--write" "${PRETTIER_ARGS[@]}") |
There was a problem hiding this comment.
The default mode when no flag is given is --write, which silently modifies files in-place. A safer default for a CI-oriented script would be --check, so that running the script without explicit flags does not mutate the working tree unexpectedly.
— Claude Code
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref_name }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
cancel-in-progress: true on the entire workflow means a new push cancels all in-flight jobs. If any of these jobs (lint, unit-tests, etc.) are required status checks in branch protection, a cancelled run counts as a failure, blocking merge until a clean run completes. Consider whether this is the intended behavior, especially with unit-tests now gated on lint via needs: lint — a cancellation during lint will cascade and cancel unit-tests too.
— Claude Code
Review by Claude Code |
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
from
eefa22b to
e3119f9
Compare
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
from
e3119f9 to
cda30ba
Compare
| run: yarn install --frozen-lockfile --network-concurrency 1 | ||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.9' |
There was a problem hiding this comment.
The unit-tests job now has needs: lint, which serializes it after the lint job. Previously unit tests and lint ran in parallel. This means a slow or flaky lint step blocks all unit tests, and contributors won't get test feedback until lint passes. This is a deliberate CI change worth calling out since the PR is framed as a prettier integration.
— Claude Code
|
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
from
cda30ba to
8cea46e
Compare
|
LGTM |
|
LGTM |
5 participants
Issue: CLDSRV-875