fix: small fixes #18127
fix: small fixes #18127
Conversation
WalkthroughThis pull request updates several CI workflows and scripts. In GitHub Action workflows, it adds a new output variable Changes
Sequence Diagram(s)sequenceDiagram
participant Prepare as Prepare Step
participant DockerBuild as Docker-Build Job
participant PostDockerBuild as Post-Docker-Build Job
participant Tests as Test Runner
Prepare->>DockerBuild: Provide MQ_SHOULD_RUN_BUILD & DOCKER_CHUNKS
alt Conditions met (MQ_SHOULD_RUN_BUILD true and DOCKER_CHUNKS not empty)
DockerBuild->>PostDockerBuild: Proceed with execution
PostDockerBuild->>Tests: Check tests outcome
alt Tests succeed
Tests-->>PostDockerBuild: Success
else Tests fail
Tests-->>PostDockerBuild: Failure (exit 1)
end
else
Note over DockerBuild,PostDockerBuild: Jobs skipped
end
sequenceDiagram
participant CI as CI Trigger
participant GenerateTag as generate-tag.mjs
participant Core as Core API
participant BranchList as MAIN_BRANCHES
CI->>GenerateTag: Trigger event with targetBranch
GenerateTag->>BranchList: Verify if targetBranch is in MAIN_BRANCHES
alt Conditions met (event is merge_group and branch included)
GenerateTag->>Core: SetOutput SHOULD_RUN_BUILD true
else
GenerateTag->>Core: SetOutput SHOULD_RUN_BUILD false
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/ci/docker/generate-tag.mjs (2)
36-39: Remove commented-out codeThe function contains commented-out code that's no longer needed, which can cause confusion.
function getTagname() { - if (eventName === 'pull_request' && context.payload.pull_request?.number) { - throw new Error(`Unsupported event: ${eventName}`) - // return `pr-${context.payload.pull_request.number}-${randomTag}`; + if (eventName === 'pull_request' && context.payload.pull_request?.number) { + throw new Error(`Unsupported event: ${eventName}`) }And similar change for
getArtifactname()function.Also applies to: 56-59
76-91: LGTM: Updated getTypeOfDeployment with MAIN_BRANCHESUsing the
MAIN_BRANCHESconstant improves maintainability. Consider removing the commented-out debug message.// UNKNOWN BRANCH - // console.error(`Unknown branch: ${targetBranch} - not sure how to tag this deployment`); throw new Error(`Unsupported branch: ${targetBranch}`).github/workflows/merge-queue.yml (2)
57-57: Conditional Execution for Docker Build Job:
Theifcondition on line 57 correctly checks that the build should run by verifying thatMQ_SHOULD_RUN_BUILDequals'true'and thatDOCKER_CHUNKSis not an empty array. Please ensure that these outputs are consistently formatted as strings so that the comparisons work as intended.
187-187: Conditional Execution in Post-Docker-Build Job:
Theifcondition on line 187 mirrors the one used in the docker-build job to decide whether to run post-docker-build. Ensure that duplicating this condition is intentional. If multiple jobs depend on the same check, consider centralizing the logic to ease future maintenance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/install.yml(2 hunks).github/workflows/merge-queue.yml(2 hunks)scripts/ci/docker/const.mjs(1 hunks)scripts/ci/docker/generate-tag.mjs(3 hunks)scripts/ci/docker/get-data.mjs(2 hunks)scripts/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- scripts/ci/docker/const.mjs
🧰 Additional context used
📓 Path-based instructions (2)
`scripts/**/*`: "Confirm that the code adheres to the follow...
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."
scripts/tsconfig.jsonscripts/ci/docker/get-data.mjsscripts/ci/docker/generate-tag.mjs
`.github/**/*`: "Confirm that the code adheres to the follow...
.github/**/*: "Confirm that the code adheres to the following:
- GitHub Actions workflows use 'arc-runners' as the valid runner for self-hosted ARC runners.
- CI pipelines are efficient, using appropriate caching strategies and minimal resource consumption.
- Reusable workflows and composite actions are properly structured for reusability.
- Dependency management workflows meet security requirements.
- Note: 'runs-on: arc-runners' is valid for our self-hosted runner configuration, despite standard linting warnings."
.github/workflows/install.yml.github/workflows/merge-queue.yml
🪛 actionlint (1.7.4)
.github/workflows/merge-queue.yml
191-191: label "arc-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (11)
scripts/tsconfig.json (1)
4-4: LGTM: Include MJS module for TypeScript awarenessIncluding the JavaScript module in the TypeScript configuration ensures TypeScript is aware of this file and can properly validate imports.
.github/workflows/install.yml (2)
22-23: LGTM: Added new workflow output variableThe addition of the
MQ_SHOULD_RUN_BUILDoutput variable follows the existing pattern and will enable conditional execution of jobs in dependent workflows.
79-79: LGTM: Properly forwarding output from step to job outputCorrectly mapping the step output to the job output, maintaining consistency with the other output variables.
scripts/ci/docker/get-data.mjs (2)
7-7: LGTM: Import MAIN_BRANCHES constantImporting the shared constant improves code maintainability and follows good modular practices.
101-101: LGTM: Replace hardcoded branch names with constantUsing the
MAIN_BRANCHESconstant instead of hardcoded branch names improves maintainability and reduces the risk of inconsistencies.scripts/ci/docker/generate-tag.mjs (3)
4-4: LGTM: Import MAIN_BRANCHES constantUsing the shared constant improves code maintainability.
17-17: LGTM: Adding SHOULD_RUN_BUILD to outputsSetting the output from the new
shouldRun()function enables conditional execution in workflows.
26-33: LGTM: New shouldRun functionThe function clearly determines when builds should execute, specifically for merge group events targeting main branches.
.github/workflows/merge-queue.yml (3)
70-70: Node Image Version Assignment Consistency:
On line 70, the job setsNODE_IMAGE_VERSIONfromneeds.pre-checks.outputs.NODE_IMAGE_VERSION. Verify that the pre-checks job reliably provides this output and that this is indeed the intended value for subsequent Docker build steps.
186-186: Post-Docker-Build Dependency Update:
Adding- testsas a dependency (line 186) ensures that the post-docker-build job waits for tests to complete. Confirm that this dependency ordering aligns with your intended CI flow and that the tests job consistently sets its result output.
193-194: Test Success Validation in Post-Docker-Build Job:
The step on lines 193–194 leverages a shell check to fail the job if tests did not pass. This is a sound approach for propagating test failures. It might be beneficial to include an explicit error message or logging for easier debugging when the condition fails.
|
View your CI Pipeline Execution ↗ for commit 3a87b2d.
☁️ Nx Cloud last updated this comment at |
* ci: docker deployment test (#18084) * chore: test * fix: test * fix: test * chore: test 2 * fix: better generate tag script * chore: test build * fix: test 2 * fix: add prepare * fix: oops * fix: wtf * fix: faster * fix: forgot * fix: ok * fix: ok * fix: wtf * fix: ok * fix: ok * fix: i hope * fix: debug * fix: ok * fix: debug * fix: wtf * fix: ok * fix: oops * fix: texst 2 * fix: debug * fix: oops * fix: export variable * fix: docker chunks * fix: chunks * fix: oops * fix: oh god * fix: oops * fix: add git branch * fix: add sha * feat: write build data * fix: upload artifact * fix: typo * fix: ok * fix: test * fix: ok * fix: use cloudposse for matrix output * feat: upload artifact * fix: write data * fix: permission * fix: test * fix: upload * fix: ok * fix: ok * fix: deploy * fix: oops * fix: okok * fix: ok * fix: ok * fix: ok * fix: ok * fix: ok * fix: fix * fix: 2 * fix: ok hehe * fix: ok changes * fix: ok * fix: ok * fix: test * fix: ok * fix: ok * fix: ok * fix: hoho * Feature/update helm values (#18100) * feat(ci): Commit helm charts to helm-values repository * feat(ci): Changing the steps order of the implementation * feat(ci): Implementing logic * feat(ci): Change implementation to use composite action --------- Co-authored-by: Ingvar Sigurðsson <[email protected]> * fix: fix fix * fix: ok * fix: ok * fix: sorry * fix: ok * fix: update manifest with bs * fix: changed files * fix: copy * fix: create directory if it does not exist * fix: test * fix: sha * fix: target * fix: ok * fix: ok * fix: ok * fix: ok * fix: test * fix: sha --------- Co-authored-by: Ingvar Sigurðsson <[email protected]> * fix: cleanup * fix: apply on mq (#18113) fiX: ok * feat: fix tag (#18118) fix: debug msg * fix: small fixes (#18127) * fix: remove test branch * fix: move yarn install * feat: get ready for release (#18136) * fix: ok * fix: ignore * fix: ok * fix: get sha * fix: ok * fix: ok * fix: ok * fix: error * fix: skip run * fix: ok * fix: merge * fix: nothing should happen (#18149) * chore: addding ssh key (#18145) * addding ssh key * fix: passing the key * fix: missed a spot * fix: test * fix: remove * fix: ok * fix: set secret * fix: test * fix: ok * fix: ok * fix: install 2 * fix: remove --------- Co-authored-by: lommi <[email protected]> * fix: update path using helm values * fix: mq-docker * chore: test prod and staging (#18160) * fix: mq-docker-main * fix: ugly hack remove this before going to main * fix: mq docker pre main * fix: mq workflow (#18162) * fix: format * fix: ok --------- Co-authored-by: Ingvar Sigurðsson <[email protected]> Co-authored-by: Róberta Andersen <[email protected]>
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Refactor