Improvement/cldsrv 875 integrate shared prettier#6124
Improvement/cldsrv 875 integrate shared prettier#6124benzekrimaha wants to merge 6 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.28% 84.24% -0.05%
===================================================
Files 206 206
Lines 13256 13256
===================================================
- Hits 11173 11167 -6
- Misses 2083 2089 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
| @@ -74,85 +78,100 @@ 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
7c921cb to
ca5e9a6
Compare
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
from
50fac01 to
58c9beb
Compare
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
from
58c9beb to
902ab8b
Compare
|
|
LGTM |
1 similar comment
|
LGTM |
| This will start the developement server, watching for changes and restarting | ||
| as needed. | ||
|
|
||
| ### Formatting |
There was a problem hiding this comment.
Then I think we should ignore package.json ?
There was a problem hiding this comment.
either add it to the ignore or we inforce the linting in it , it's a choice to make we can check with others what they prefer
| "ft_sse_arn": "cd tests/functional/sse-kms-migration && mocha --reporter mocha-multi-reporters --reporter-options configFile=$INIT_CWD/tests/reporter-config.json -t 10000 cleanup.js arnPrefix.js --exit", | ||
| "lint": "eslint $(git ls-files '*.js')", | ||
| "lint_md": "mdlint $(git ls-files '*.md')", | ||
| "format:changed": "scripts/format-changed.sh", |
There was a problem hiding this comment.
| "format:changed": "scripts/format-changed.sh", | |
| "format:check:diff": "scripts/format-changed.sh", |
Maybe ?
There was a problem hiding this comment.
tried to keep the same name as the sh , can indeed be changed
There was a problem hiding this comment.
the script can be renamed as well
There was a problem hiding this comment.
Maybe a quick comment to explain the goal of the script and how it's working ?
|
LGTM |
There was a problem hiding this comment.
large reformatting for a few deleted lines make this very difficult to review...
There was a problem hiding this comment.
I’ve reduced this to a minimal, targeted change in the workflow (no broad formatting pass).
In tests.yaml , only the intended lines were kept:
- concurrency group simplification
- build-args switched to multiline block format
- No unrelated reformatting remains.
| "ft_sse_arn": "cd tests/functional/sse-kms-migration && mocha --reporter mocha-multi-reporters --reporter-options configFile=$INIT_CWD/tests/reporter-config.json -t 10000 cleanup.js arnPrefix.js --exit", | ||
| "lint": "eslint $(git ls-files '*.js')", | ||
| "lint_md": "mdlint $(git ls-files '*.md')", | ||
| "format:changed": "scripts/format-changed.sh", |
There was a problem hiding this comment.
the script can be renamed as well
benzekrimaha
force-pushed
the
improvement/CLDSRV-875-Integrate-shared-prettier
branch
from
16e46ef to
d8df099
Compare
There was a problem hiding this comment.
do not use a separate workflow : this just creates noise (more runs in actions, bert-e not properly displaying the build status...) and bugs (inconsistent triggers or concurrency).
if you really want to split the file, just make the "main" workflow invoke the other:
- use workflow_call trigger
on: workflow_call: - create a job in the regular "tests" workflow which uses that workflow
lint: uses: ./.github/workflows/lint.yaml secrets: inherit
| cd "$ROOT_DIR" | ||
|
|
||
| BASE_SHA="${BASE_SHA:-}" | ||
| BASE_REF="${BASE_REF:-${GITHUB_BASE_REF:-}}" |
There was a problem hiding this comment.
In CI, BASE_REF is GITHUB_BASE_REF. No need for 2 variables, can use GITHUB_BASE_REF directly.
| set -euo pipefail | ||
|
|
||
| # Purpose: | ||
| # - Run Prettier on files changed in the current branch/PR. |
There was a problem hiding this comment.
when running locally, there is a semantic question here: what do we mean by diff ?
(basically, this is the same question as git diff)
It can mean:
- the diff in the local workspace (easiest to implement and to reason with)
- the diff from the "upstream" branch (i.e. everything local or committed that is not in git upstream branch: which can be both the remote branch -what we push to- or the "parent" branch when stacking PRs)
- the diff from the development branch we based our work on and will eventually push to
- ...
To keep things simple, instead of choosing either or trying to automatically cover every case (and likely adding lot of complexity and more coupling) I would rather just rely on git diff (and allow passing params to git diff) : maximum flexibility and minimum work in the script...
| if [[ -n "$ORIGIN_HEAD" ]]; then | ||
| DEFAULT_BASE="${DEFAULT_BASE:-$ORIGIN_HEAD}" | ||
| else | ||
| DEFAULT_BASE="${DEFAULT_BASE:-origin/development/9.3}" |
There was a problem hiding this comment.
do not hard-code a branch. If we can't figure it out, just bail out...
| exit 1 | ||
| fi | ||
|
|
||
| mapfile -t CHANGED < <(git diff --name-only "${DIFF_BASE}..HEAD" --diff-filter=ACMRT \ |
There was a problem hiding this comment.
I suggest removing the logic above, and just simplify this to something like git diff --name-only "$@"
(will need to use shift after processing our own params)
→ can be used for most use-case locally
→ can directly pass ${{ github.event.pull_request.base.sha }}..HEAD from the CI workflow (less coupling)
5 participants
Issue: CLDSRV-875