Revert "ci: making it easier to upgrade node" #14967
Conversation
This reverts commit 3527664.
WalkthroughThe overall changes focus on simplifying the GitHub Actions workflows, updating Node.js versions, and refining Docker configurations. Key updates include reorganizing conditional logic for event handling, downgrading Changes
Tip Early Access Features
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (4)
infra/scripts/Dockerfile (2)
Line range hint
10-10: ReplaceADDwithCOPYfor better clarity and becauseCOPYis recommended overADDunless you need to auto-extract a tarball.- ADD infra/package.json infra/yarn.lock /app/infra/ - ADD infra/ /app/infra/ - ADD apps /app/apps/ - ADD libs /app/libs/ + COPY infra/package.json infra/yarn.lock /app/infra/ + COPY infra/ /app/infra/ + COPY apps /app/apps/ + COPY libs /app/libs/Also applies to: 15-17
Line range hint
20-20: Consider consolidating multipleRUNinstructions into a single layer to reduce the number of layers in the image, which can help optimize the build process.- RUN ./node_modules/.bin/ncc build src/feature-env.ts -o /app/dist/feature-env - RUN ./node_modules/.bin/ncc build src/secrets.ts -o /app/dist/secrets + RUN ./node_modules/.bin/ncc build src/feature-env.ts -o /app/dist/feature-env && \ + ./node_modules/.bin/ncc build src/secrets.ts -o /app/dist/secretsscripts/ci/Dockerfile (2)
Line range hint
11-17: ReplaceADDwithCOPYfor better clarity and becauseCOPYis recommended overADDunless you need to auto-extract a tarball.- ADD package.json yarn.lock .yarnrc.yml ./ - ADD apps/native/app/package.json ./apps/native/app/ + COPY package.json yarn.lock .yarnrc.yml ./ + COPY apps/native/app/package.json ./apps/native/app/
Line range hint
19-19: Use the--progress=dot:gigaoption withwgetto enable a more compact progress bar.- RUN wget -O /tmp/jq-linux64 https://github.com/stedolan/jq/releases/download/jq-1.6/jq-linux64 + RUN wget --progress=dot:giga -O /tmp/jq-linux64 https://github.com/stedolan/jq/releases/download/jq-1.6/jq-linux64
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- .github/actions/main.ts (1 hunks)
- .github/actions/package.json (1 hunks)
- .github/actions/tsconfig.json (1 hunks)
- .github/actions/unit-test/action.yml (1 hunks)
- .github/actions/yarn.lock (2 hunks)
- .github/workflows/config-values.yaml (2 hunks)
- .github/workflows/pullrequest.yml (8 hunks)
- .github/workflows/push.yml (14 hunks)
- infra/scripts/Dockerfile (1 hunks)
- scripts/ci/10_prepare-docker-deps.sh (2 hunks)
- scripts/ci/Dockerfile (2 hunks)
Files skipped from review due to trivial changes (3)
- .github/actions/package.json
- .github/workflows/config-values.yaml
- scripts/ci/10_prepare-docker-deps.sh
Additional Context Used
Biome (12)
.github/actions/main.ts (12)
8-8: Forbidden non-null assertion.
8-8: Forbidden non-null assertion.
17-17: Forbidden non-null assertion.
18-18: Forbidden non-null assertion.
19-19: Forbidden non-null assertion.
20-20: Forbidden non-null assertion.
26-26: Forbidden non-null assertion.
27-27: Forbidden non-null assertion.
28-28: Forbidden non-null assertion.
32-32: Do not use template literals if interpolation and special-character handling are not needed.
4-6: All these imports are only used as types.
8-8: This let declares a variable that is only assigned once.
Hadolint (30)
infra/scripts/Dockerfile (10)
4: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
4: Use the
--no-cacheswitch to avoid the need to use--updateand remove/var/cache/apk/*when done installing packages
10: Use COPY instead of ADD for files and folders
11:
COPYto a relative destination withoutWORKDIRset.
15: Use COPY instead of ADD for files and folders
16: Use COPY instead of ADD for files and folders
17: Use COPY instead of ADD for files and folders
20: Multiple consecutive
RUNinstructions. Consider consolidation.
25:
COPYto a relative destination withoutWORKDIRset.
26:
COPYto a relative destination withoutWORKDIRset.scripts/ci/Dockerfile (20)
6: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
6: Use the
--no-cacheswitch to avoid the need to use--updateand remove/var/cache/apk/*when done installing packages
11: Use COPY instead of ADD for files and folders
12: Use COPY instead of ADD for files and folders
14: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
15:
yarn cache cleanmissing afteryarn installwas run.
19: Avoid use of wget without progress bar. Use
wget --progress=dot:giga <url>. Or consider using-qor-nv(shorthands for--quietor--no-verbose).
24: Use COPY instead of ADD for files and folders
56: Pin versions in npm. Instead of
npm install <package>usenpm install <package>@<version>
100: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
100: Use the
--no-cacheswitch to avoid the need to use--updateand remove/var/cache/apk/*when done installing packages
104: Use COPY instead of ADD for files and folders
105: Use COPY instead of ADD for files and folders
106: Use COPY instead of ADD for files and folders
113: Multiple consecutive
RUNinstructions. Consider consolidation.
113: Pin versions in npm. Instead of
npm install <package>usenpm install <package>@<version>
132: Avoid additional packages by specifying
--no-install-recommends
132: Pin versions in apt get install. Instead of
apt-get install <package>useapt-get install <package>=<version>
144: Use COPY instead of ADD for files and folders
165:
COPYto a relative destination withoutWORKDIRset.
Path-based Instructions (1)
scripts/ci/Dockerfile (1)
Pattern
scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
Additional comments not posted (8)
.github/actions/tsconfig.json (1)
15-15: Change in TypeScript configuration aligns with the objective to simplify and stabilize the build environment..github/actions/main.ts (1)
11-29: Simplified conditional logic for determining the revision based on the GitHub event type aligns with the PR's objective to simplify logic..github/actions/unit-test/action.yml (1)
39-41: Updated Node.js version and setup-node action version to ensure consistency across workflows.Verification successful
The verification confirms that the Node.js version '18' is consistently used across multiple workflow files, including
.github/workflows/push.yml,.github/workflows/pullrequest.yml, and.github/workflows/config-values.yaml.
.github/workflows/push.yml.github/workflows/pullrequest.yml.github/workflows/config-values.yamlScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Node.js version is consistent across all workflow files. # Test: Search for Node.js version across workflows. Expect: Consistent version. rg --type yaml $'node-version: \\'18\\''Length of output: 237
Script:
#!/bin/bash # Description: Verify that the Node.js version is consistent across all workflow files. # Test: Search for Node.js version across workflows. Expect: Consistent version. rg --type yaml 'node-version: .?18'Length of output: 37
Script:
#!/bin/bash # Description: Verify all instances of Node.js version across workflow files. # Test: Search for all 'node-version' entries in YAML files to check for inconsistencies. rg --type yaml 'node-version:'Length of output: 30
Script:
#!/bin/bash # Description: List all YAML files in the repository to manually inspect for 'node-version' entries. # List all YAML files in the repository. fd --type f --extension yamlLength of output: 3027
Script:
#!/bin/bash # Description: Verify all instances of Node.js version in GitHub Actions workflow files. # Search for all 'node-version' entries in the .github/workflows directory. rg --type yaml 'node-version:' .github/workflowsLength of output: 853
.github/workflows/push.yml (3)
369-369: Ensure the script10_prepare-docker-deps.shis optimized for performance and error handling.Verification successful
The script
10_prepare-docker-deps.shis well-optimized for performance and includes robust error handling mechanisms.
- The use of
set -euxo pipefailensures that the script exits on errors, treats unset variables as errors, and fails on any command in a pipeline.- The script employs Docker buildx with caching, which enhances performance by reusing previously built layers.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the contents of the script for performance and error handling practices. # Test: Display the script content. Expect: Proper error handling and performance optimizations. cat scripts/ci/10_prepare-docker-deps.shLength of output: 693
604-606: Review the use ofrun-in-parallel.shfor building Docker images to ensure it handles errors and parallel execution correctly.Also applies to: 608-612
Verification successful
The
run-in-parallel.shscript correctly handles errors and parallel execution:
- It uses
set -euo pipefailto ensure strict error handling.- It utilizes
parallelwith the--halt soon,fail=1option to stop all jobs if one fails, ensuring robust parallel execution.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the script for error handling and parallel execution capabilities. # Test: Display the script content. Expect: Proper error handling and support for parallel execution. cat scripts/ci/run-in-parallel.shLength of output: 620
451-453: Confirm that the Node.js version '18' is appropriate for all test jobs..github/actions/yarn.lock (2)
1613-1618: The downgrade of@types/nodeto20.11.4and the associated dependency updates are correctly reflected.
1718-1718: The version of@types/nodeis consistently set to20.11.4across the file, aligning with the revert objective.
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: '18' |
There was a problem hiding this comment.
Duplicate actions/setup-node steps detected. Consider merging them to simplify the workflow.
- - uses: actions/setup-node@v3
- with:
- node-version: '18'
- if: steps.gather.outcome == 'success'
- - uses: actions/setup-node@v3
- with:
- node-version: '18'
- if: steps.gather.outcome == 'success'
+ - uses: actions/setup-node@v3
+ with:
+ node-version: '18'
+ if: steps.gather.outcome == 'success'Also applies to: 523-525
Committable suggestion was skipped due low confidence.
| PACKAGE_JSON_HASH=$(cat package.json | jq '{resolutions,dependencies,devDependencies}' | sha1sum -t | cut -f1 -d" ") | ||
| echo "PACKAGE_JSON_HASH: $PACKAGE_JSON_HASH" | ||
| export NODE_MODULES_HASH=${{ runner.os }}-${{ hashFiles('yarn.lock') }}-$PACKAGE_JSON_HASH | ||
| echo "NODE_MODULES_HASH: $NODE_MODULES_HASH" | ||
| echo "node-modules-hash=$NODE_MODULES_HASH" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Optimize the calculation of NODE_MODULES_HASH by using a more efficient hashing mechanism.
- PACKAGE_JSON_HASH=$(cat package.json | jq '{resolutions,dependencies,devDependencies}' | sha1sum -t | cut -f1 -d" ")
- export NODE_MODULES_HASH=${{ runner.os }}-${{ hashFiles('yarn.lock') }}-$PACKAGE_JSON_HASH
+ export NODE_MODULES_HASH=${{ runner.os }}-${{ hashFiles('package.json', 'yarn.lock') }}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| PACKAGE_JSON_HASH=$(cat package.json | jq '{resolutions,dependencies,devDependencies}' | sha1sum -t | cut -f1 -d" ") | |
| echo "PACKAGE_JSON_HASH: $PACKAGE_JSON_HASH" | |
| export NODE_MODULES_HASH=${{ runner.os }}-${{ hashFiles('yarn.lock') }}-$PACKAGE_JSON_HASH | |
| echo "NODE_MODULES_HASH: $NODE_MODULES_HASH" | |
| echo "node-modules-hash=$NODE_MODULES_HASH" >> $GITHUB_OUTPUT | |
| export NODE_MODULES_HASH=${{ runner.os }}-${{ hashFiles('package.json', 'yarn.lock') }} | |
| echo "NODE_MODULES_HASH: $NODE_MODULES_HASH" | |
| echo "node-modules-hash=$NODE_MODULES_HASH" >> $GITHUB_OUTPUT |
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version-file: 'package.json' | ||
| node-version: '18.8.0' |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure consistent Node.js version across all workflow jobs.
- The following files have inconsistent Node.js versions:
.github/workflows/push.yml.github/workflows/pullrequest.yml
Please update all instances to use the same Node.js version, preferably 18.8.0.
Analysis chain
Ensure the Node.js version is consistent across all workflow jobs.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all instances of `actions/setup-node` use the same Node.js version.
# Test: Search for different Node.js versions. Expect: Only '18.8.0' should be used.
rg --type yaml $'actions/setup-node@v3' .github/workflows/*.yml | rg 'node-version'
Length of output: 304
Script:
#!/bin/bash
# Description: Verify that all instances of `actions/setup-node` use the same Node.js version.
# Test: Search for all instances of `node-version` in the workflow files.
rg --type yaml 'node-version' .github/workflows/*.yml
Length of output: 950
| @@ -1,10 +1,9 @@ | |||
| ARG DOCKER_IMAGE_REGISTRY=public.ecr.aws | |||
| ARG NODE_IMAGE_TAG | |||
| FROM ${DOCKER_IMAGE_REGISTRY}/docker/library/node:${NODE_IMAGE_TAG} as runner | |||
| FROM $DOCKER_IMAGE_REGISTRY/docker/library/node:18-alpine3.15 as runner | |||
|
|
|||
| RUN apk add postgresql-client bash | |||
There was a problem hiding this comment.
Use the --no-cache option with apk add to avoid unnecessary cache storage.
- RUN apk add postgresql-client bash
+ RUN apk add --no-cache postgresql-client bashCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| RUN apk add postgresql-client bash | |
| RUN apk add --no-cache postgresql-client bash |
Consider pinning the version of packages installed with apk add to ensure reproducibility.
- RUN apk add postgresql-client bash
+ RUN apk add postgresql-client=<version> bash=<version>Committable suggestion was skipped due low confidence.
| `'${process.env.HEAD_REF!}'`, | ||
| `'${process.env.BASE_REF!}'`, | ||
| process.env.WORKFLOW_ID! as WorkflowID, | ||
| ) | ||
|
|
||
| if (rev === 'rebuild') { | ||
| console.log(`Full rebuild needed`) |
There was a problem hiding this comment.
Avoid using template literals when not necessary for interpolation or special characters.
- console.log(`Full rebuild needed`)
+ console.log('Full rebuild needed')Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| console.log(`Full rebuild needed`) | |
| console.log('Full rebuild needed') |
| diffWeight, | ||
| git, | ||
| runner, | ||
| `'${process.env.HEAD_REF!}'`, |
There was a problem hiding this comment.
Avoid using non-null assertions unless absolutely necessary as they bypass TypeScript's type safety checks.
- `'${process.env.HEAD_REF!}'`
- `'${process.env.BASE_REF!}'`
- `'${process.env.PR_REF!}'`
+ process.env.HEAD_REF ?? 'default-value'
+ process.env.BASE_REF ?? 'default-value'
+ process.env.PR_REF ?? 'default-value'Also applies to: 18-18, 19-19, 26-26, 27-27
Committable suggestion was skipped due low confidence.
| @@ -1,8 +1,7 @@ | |||
| # This is a multi-stage Dockerfile which contains all CI-related operations as well as images to be deployed in production | |||
| ARG PLAYWRIGHT_VERSION | |||
| ARG DOCKER_IMAGE_REGISTRY=public.ecr.aws | |||
| ARG NODE_IMAGE_TAG | |||
| FROM ${DOCKER_IMAGE_REGISTRY}/docker/library/node:${NODE_IMAGE_TAG} as deps | |||
| FROM $DOCKER_IMAGE_REGISTRY/docker/library/node:18-alpine3.15 as deps | |||
There was a problem hiding this comment.
Consider pinning the version of packages installed with apk add to ensure reproducibility.
- RUN apk add -U git
+ RUN apk add -U git=<version>Committable suggestion was skipped due low confidence.
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version-file: 'package.json' | ||
| node-version: '18.8.0' |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process revealed that there are multiple instances of node-version in the .github/workflows directory, with versions 18.8.0 and 18 being used. This indicates inconsistency in the Node.js versions across the workflow files.
-
.github/workflows/push.yml:node-version: '18.8.0'node-version: '18'(multiple instances)
-
.github/workflows/pullrequest.yml:node-version: '18.8.0'node-version: '18'(multiple instances)
To ensure consistency, it is recommended to standardize the Node.js version across all workflow files.
Analysis chain
Updated Node.js version and setup-node action version to ensure consistency across workflows.
Also applies to: 249-251, 310-312, 373-375, 416-418, 460-462, 509-511
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Node.js version is consistent across all workflow files.
# Test: Search for Node.js version across workflows. Expect: Consistent version.
rg --type yaml $'node-version: \\'18\\''
Length of output: 237
Script:
#!/bin/bash
# Description: Verify that the Node.js version is consistent across all workflow files.
# Test: Search for Node.js version across workflows. Expect: Consistent version.
rg --type yaml 'node-version: '\''18.8.0'\'''
Length of output: 47
Script:
#!/bin/bash
# Description: Search for all instances of `node-version` in the workflow files to identify any discrepancies.
# Test: Search for `node-version` across workflows.
rg --type yaml 'node-version'
Length of output: 29
Script:
#!/bin/bash
# Description: List all YAML files in the repository to manually inspect for `node-version`.
# List all YAML files in the repository.
fd --type f --extension yaml
Length of output: 3027
Script:
#!/bin/bash
# Description: Search for `node-version` specifically within the `.github/workflows` directory to identify any discrepancies.
# Test: Search for `node-version` in the `.github/workflows` directory.
rg --glob '.github/workflows/*.yml' 'node-version'
Length of output: 199
Script:
#!/bin/bash
# Description: List the contents of the `.github/workflows` directory and search for `node-version` within those files.
# List the contents of the `.github/workflows` directory.
echo "Contents of .github/workflows directory:"
ls .github/workflows
# Search for `node-version` within the `.github/workflows` directory.
echo "Searching for 'node-version' in .github/workflows/*.yml files:"
rg 'node-version' .github/workflows/*.yml
Length of output: 1334
Reverts #14472
Summary by CodeRabbit
New Features
Refactor
18-alpine3.15) instead of variable references.Chores
@types/nodefrom20.12.12to20.11.4for consistency.es2019andesnext.array).