Improvement/cldsrv 875 integrate shared prettier#6124
Improvement/cldsrv 875 integrate shared prettier#6124benzekrimaha wants to merge 9 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.28%
================================================
Files 206 206
Lines 13256 13256
================================================
Hits 11173 11173
Misses 2083 2083
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 |
|
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 |
| uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
these are not the latest actions: upgrade instead of introducing old dependency
There was a problem hiding this comment.
large reformatting for a few deleted lines make this very difficult to review...
| if ! git rev-parse --verify "${DIFF_BASE}^{commit}" >/dev/null 2>&1; then | ||
| if [[ -n "$BASE_REF" ]]; then | ||
| git fetch --no-tags --prune --depth=50 origin "${BASE_REF#origin/}" >/dev/null 2>&1 || true | ||
| else | ||
| git fetch --no-tags --prune --depth=50 origin "${DIFF_BASE#origin/}" >/dev/null 2>&1 || true | ||
| fi | ||
| fi |
There was a problem hiding this comment.
- instead of re-fecthing, best to set proper parameters in the workflow, during checkout
- "50" is completely arbitrary, and may not cover the whole range anyway... best to just pull the whole history (in the workflow)
| "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
| "format": "prettier --write .", | ||
| "format:check": "prettier --check .", |
There was a problem hiding this comment.
do we really all need these? (not many, but still quite redundant)
just a single prettier command is probably enough, and developer can pass additional flags to yarn : yarn run prettier --check ?
There was a problem hiding this comment.
and similarly the --check could be removed from the script, and extra args added to prettier call:
yarn run --silent prettier "${CHANGED[@]}" "$@"
so we have a generic prettier:diff command, which can be called either for reformatting or just checking:
yarn run prettier:diff --check
yarn run prettier:diff --format
| contents: read | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name || github.sha }} |
There was a problem hiding this comment.
github.head_ref and github.ref_name are always set for pull_request, no point making things more complicated
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name || github.sha }} | |
| group: ${{ github.workflow }}-${{ github.head_ref }} |
| - name: Lint Yaml | ||
| run: yamllint -c yamllint.yml $(git ls-files "*.yml") | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name || github.sha }} |
There was a problem hiding this comment.
github.head_ref is never set on push, and github.ref_name is always set, no point making things more complicated
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name || github.sha }} | |
| group: ${{ github.workflow }}-${{ github.ref_name }}`` |
| provenance: false | ||
| build-args: | ||
| CLOUDSERVER_VERSION=${{ github.sha }} | ||
| build-args: CLOUDSERVER_VERSION=${{ github.sha }} |
There was a problem hiding this comment.
I think this was intentional, to clearly show each value
| build-args: CLOUDSERVER_VERSION=${{ github.sha }} | |
| build-args: | | |
| CLOUDSERVER_VERSION=${{ github.sha }} |
|
|
||
| BASE_SHA="${BASE_SHA:-}" | ||
| BASE_REF="${BASE_REF:-${GITHUB_BASE_REF:-}}" | ||
| DEFAULT_BASE="${DEFAULT_BASE:-origin/main}" |
There was a problem hiding this comment.
main is not an existing branch in this repository
5 participants
Issue: CLDSRV-875