CI | seperate noobaa image build to different action #9051
Conversation
nadavMiz
force-pushed
the
ci-build-noobaa
branch
2 times, most recently
from
ddf51c5 to
2e5bdf1
Compare
nadavMiz
force-pushed
the
ci-build-noobaa
branch
23 times, most recently
from
9387673 to
2a39817
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/ceph-s3-tests.yaml (1)
9-9: Review permissions scope
Duplicate of previous suggestion:permissions: read-allis very broad—limit it to only what’s required..github/workflows/warp-nc-tests.yaml (1)
10-10: Review permissions scope
permissions: read-allcould be scoped down to only the required API permissions..github/workflows/ceph-nsfs-s3-tests.yaml (1)
8-8: Review permissions scope
As before, consider narrowingpermissions: read-allto just what's needed for artifact download and Docker load.
🧹 Nitpick comments (4)
.github/workflows/unit.yaml (1)
19-19: Remove trailing whitespace
Delete the extra spaces on this blank line to satisfy YAML lint.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
.github/workflows/postgres-unit-tests.yaml (1)
9-9: Review permissions scope
You’ve addedpermissions: read-all, which is broad—consider locking this down to only the necessary scopes (e.g.actions: read,contents: read,checks: read,artifacts: read) for least privilege..github/workflows/ceph-nsfs-s3-tests.yaml (1)
31-31: Add a newline at end of file
YAML linter flagged a missing newline; please ensure the file ends with a single newline character.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/run-pr-tests.yaml (1)
54-58: Save Docker images as artifacts
Serializing bothnoobaaandnoobaa-testerinto tarballs works, but consider using GitHub’s Container Registry to host and pull images instead of tar artifacts to reduce overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
.github/workflows/ceph-nsfs-s3-tests.yaml
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-package-lock-validation
- GitHub Check: build-noobaa-image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (60)
.github/workflows/nc_unit.yml (6)
2-2: Trigger updated toworkflow_call
Converted this workflow to a reusable action correctly.
8-8: Permissions set toread-all
Appropriate replacement for concurrency controls.
10-10: Checkout step remains correct
The renamed checkout step still usesactions/checkout@v4correctly.
13-17: Artifact download step is valid
Properly downloads thenoobaa-testerartifact to/tmp.
19-20: Docker image load step is correct
Loads the downloaded tarball as a Docker image.
24-24: Test command uses-o testerflag
Ensures tests run against the pre-built tester image..github/workflows/sanity.yaml (5)
2-2: Trigger updated toworkflow_call
Workflow is now reusable.
8-8: Permissions set toread-all
Replaces concurrency controls appropriately.
13-17: Artifact download step is valid
Correctly retrieves thenoobaa-testerartifact to/tmp.
19-20: Docker image load step is correct
Loads the pre-built tester image without rebuilding.
27-27: Sanity test invocation is correct
make test-sanity -o testeraligns with the new image usage..github/workflows/warp-tests.yaml (5)
2-2: Trigger updated toworkflow_call
Converted to a reusable workflow entry point.
8-8: Permissions set toread-all
Consistent with other test workflows.
16-20: Artifact download step is valid
Downloads thenoobaa-testerartifact into/tmpas expected.
22-23: Docker image load step is correct
Restores the tester image for subsequent test runs.
36-36: Warp test invocation is correct
make test-warp -o testerensures tests use the pre-built image..github/workflows/sanity-ssl.yaml (4)
2-2: Trigger updated toworkflow_call
Workflow is now a callable reusable action.
12-16: Artifact download step is valid
Properly downloadsnoobaa-testerto/tmp.
18-19: Docker image load step is correct
Loads the tester image for SSL sanity tests.
26-26: SSL sanity test invocation is correct
make test-external-pg-sanity -o testeraligns with the new image usage..github/workflows/unit.yaml (6)
2-2: Trigger updated toworkflow_call
Reusable workflow entry point is correct.
6-6: Job named explicitly
The explicitname: run-unit-testsaligns with other workflows.
9-9: Permissions set toread-all
Replaces concurrency controls appropriately.
14-18: Artifact download step is valid
Correctly retrieves thenoobaa-testerartifact to/tmp.
20-21: Docker image load step is correct
Loads the pre-built tester image without rebuilding.
25-25: Primary test invocation is correct
make test -o testeris aligned with new workflow conventions..github/workflows/postgres-unit-tests.yaml (4)
2-2: Convert to a reusable workflow trigger
Changing the trigger toworkflow_callmakes this workflow callable by other workflows.
14-18: Download the NooBaa tester artifact
The new artifact-download step is correct and ensures the downstream job uses the pre-built image.
20-21: Load the Docker image from the artifact
Loading the tar directly into Docker is the right approach for reusing the tester image.
24-24: Pass the tester flag to the test runner
Appending-o testerensures the tests run against the downloaded tester image..github/workflows/ceph-s3-tests.yaml (5)
2-2: Convert to a reusable workflow trigger
Swapping toon: [workflow_call]makes this workflow callable by the orchestrator.
6-6: Explicitly name the job
Definingname: ceph-s3-testsclarifies the purpose in the UI.
17-21: Download the NooBaa tester artifact
These steps mirror the other test workflows and correctly pull in the prebuilt image.
23-24: Load the Docker image from the artifact
Thedocker loadcommand here correctly restores the tester image.
32-32: Pass the tester flag to the test target
Adding-o testertomake test-cephs3aligns it with the orchestrated workflow..github/workflows/warp-nc-tests.yaml (5)
3-3: Convert to a reusable workflow trigger
Switching toon: [workflow_call]allows this to be invoked by the main orchestrator.
7-7: Explicitly name the job
Havingname: warp-nc-testsmakes the workflow more readable in GH Actions.
18-22: Download the NooBaa tester artifact
Correctly pulls the tester image tar into/tmpfor loading.
24-25: Load the Docker image from the artifact
Properly restores the image before running NC warp tests.
38-38: Pass the tester flag to the NC warp tests
Including-o testeronmake test-nc-warpis consistent with other workflows..github/workflows/ceph-nsfs-s3-tests.yaml (5)
2-2: Convert to a reusable workflow trigger
Moving toon: [workflow_call]makes this test suite reusable by the orchestrator.
10-14: Simplify and standardize the checkout step
Renaming toCheckoutand removing quotes aroundrepositorymakes this consistent with the other workflows.
16-20: Download the NooBaa tester artifact
These steps correctly fetch the tester image for NSFS Ceph S3 testing.
22-23: Load the Docker image from the artifact
docker load --input /tmp/noobaa-tester.tarrestores the tester image as expected.
31-31: Pass the tester flag to NSFS Ceph S3 tests
Adding-o testerto your make target ensures consistency with the orchestrated pipeline.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/run-pr-tests.yaml (15)
1-2: Define the orchestrator workflow
Settingname: Run PR Testsand triggering on push/pull_request/workflow_dispatch is correct for entry points.
3-5: Enable concurrency control
Cancelling in-progress runs on the same ref prevents redundant builds—good addition.
8-11: Invoke sanity tests as a reusable workflow
needs: build-noobaa-imageanduses: ./.github/workflows/sanity.yamlcorrectly wire up dependencies.
12-15: Invoke SSL sanity tests as a reusable workflow
Consistent with the other test jobs.
16-19: Invoke general unit tests as a reusable workflow
This ensures your standard unit suite runs against the prebuilt image.
20-23: Invoke Postgres-specific unit tests as a reusable workflow
Good separation of database-specific tests.
24-27: Invoke NC unit tests as a reusable workflow
Maintains modularity by calling the dedicated NC workflow.
28-31: Invoke Ceph S3 tests as a reusable workflow
Correctly wired to depend on the image build.
32-35: Invoke NSFS Ceph S3 tests as a reusable workflow
Matches the other Ceph test invocations.
36-39: Invoke general Warp tests as a reusable workflow
Consistent pattern for all test categories.
40-43: Invoke NC Warp tests as a reusable workflow
Follows the same design for NC-specific Warp tests.
44-47: Build the NooBaa tester Docker image
Thebuild-noobaa-imagejob clearly encapsulates the build step with proper timeout.
48-52: Checkout code and build tester image
Usingactions/checkout@v4followed bymake testerproperly constructs the image.
59-64: Upload the NooBaa image artifact
Retention of one day is sufficient; artifact setup looks correct.
66-71: Upload the NooBaa tester image artifact
This matches the artifact name consumed by downstream workflows.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
5fd5630 to
51c8f90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ceph-nsfs-s3-tests.yaml (1)
16-23: Validate artifact consumption steps
Ensure the artifact namenoobaa-testerand path/tmpmatch the build workflow output, and verifydocker loaduses the correct file.
🧹 Nitpick comments (2)
.github/workflows/unit.yaml (1)
19-19: Remove trailing whitespace
There are trailing spaces on this line; please remove them to satisfy YAML linting.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
.github/workflows/ceph-nsfs-s3-tests.yaml (1)
31-31: Add newline at end of file
Please ensure there is a trailing newline character to satisfy YAML formatting conventions.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/warp-tests.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/sanity-ssl.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ceph-nsfs-s3-tests.yaml
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/unit.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-noobaa-image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (8)
.github/workflows/unit.yaml (4)
2-2: Switch to reusable workflow trigger
Usingworkflow_callis correct to make this a reusable workflow.
6-6: Explicit job naming
Therun-unit-testsjob name is clear and aligns with its purpose.
9-9: Set job permissions appropriately
Usingpermissions: read-allis necessary for artifact download.
14-21: Validate artifact download and image load
Confirm that thenoobaa-testerartifact tarball is correctly downloaded to/tmp/noobaa-tester.tarand thatdocker loadpoints to the correct location.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
.github/workflows/ceph-nsfs-s3-tests.yaml (4)
2-2: Switch to reusable workflow trigger
Usingworkflow_callensures this workflow can be invoked from the orchestrator.
8-8: Set job permissions appropriately
Grantingread-allis required for artifact download and image loading.
10-14: Simplify checkout step
The updated checkout step correctly specifies therepositoryand clean path without unnecessary quotes.
31-31: Include tester option in test command
The-o testerflag correctly instructs the test suite to use the downloaded tester image.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
51c8f90 to
4213fef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/unit.yaml (2)
19-19: Remove trailing whitespace.
Line 19 contains unnecessary spaces – trimming them improves YAML lint.- path: /tmp + path: /tmp🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
27-27: Eliminate extra blank line.
YAMLLint flags too many blank lines; remove the empty line at the end for consistency.-🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: build-noobaa-image
🔇 Additional comments (6)
.github/workflows/unit.yaml (6)
2-2: Confirmed trigger update toworkflow_call.
Switching frompush/pull_requesttoworkflow_callis correct for a reusable workflow.
6-6: Job name made explicit.
Renaming the job torun-unit-testsimproves readability in the workflow UI.
9-9: Permissions set toread-all.
This aligns with other workflows and ensures artifact download and checkout permissions.
14-18: Artifact download step added.
Pulling thenoobaa-testerimage here matches the new orchestrator workflow. Ensure the artifact name and path (/tmp) align with the build step in yourrun-pr-tests.yaml.
21-21: Load Docker image from artifact.
docker load --input /tmp/noobaa-tester.tarcorrectly restores the tester image.
25-26: Test commands updated to use tester image.
Adding-o testerto bothmake testandmake root-perm-testensures the prebuilt image is used.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
2 times, most recently
from
027a13f to
de82d75
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/unit.yaml (5)
2-2: Prefer mapping syntax for triggers
Whileon: [workflow_call]is valid shorthand, consider using the mapping style for clarity and to easily add inputs later:-on: [workflow_call] +on: + workflow_call:
6-6: Jobnameduplicates its ID; consider human-friendly naming
Thenamefield mirrors the job IDrun-unit-tests. You can omit it or use a more descriptive title, e.g.:-name: run-unit-tests +name: Run Unit Tests
14-18: Use runner-provided temp directory instead of hard-coded/tmp
Replace/tmpwith${{ runner.temp }}for portability and to follow best practices:- - name: Download artifact - uses: actions/download-artifact@v4 - with: - name: noobaa-tester - path: /tmp + - name: Download artifact + uses: actions/download-artifact@v4 + with: + name: noobaa-tester + path: ${{ runner.temp }}
19-19: Remove trailing whitespace
Line 19 contains trailing spaces—trim them to satisfy linting rules:🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
27-27: Remove extra blank line
The extra blank line at the end of the file is flagged by linting—remove it to adhere to file formatting guidelines.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
🛑 Comments failed to post (1)
.github/workflows/unit.yaml (1)
9-9: 🛠️ Refactor suggestion
Restrict permissions to principle of least privilege
Usingread-allgrants broad access. Audit the actual scopes needed (e.g.,contents: read,packages: read) and narrow this down to minimize risk.🤖 Prompt for AI Agents
In .github/workflows/unit.yaml at line 9, the permissions setting uses 'read-all', which grants overly broad access. Review the workflow's actual needs and replace 'read-all' with the minimal required scopes such as 'contents: read' or 'packages: read' to adhere to the principle of least privilege and reduce security risks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/unit.yaml (4)
2-2: Declare workflow inputs for clarity.Since this is a reusable workflow triggered by
workflow_call, consider defininginputs:underon: workflow_call(e.g., artifact name, tester tag) to make the expected parameters explicit.
9-9: Scope down permissions.Avoid
permissions: read-all; specify only needed scopes (e.g.,actions: read,contents: read) to adhere to least privilege.
20-21: Validate Docker load input.Consider adding a preliminary check (e.g.,
ls /tmp/noobaa-tester.tar) to fail fast with a clear message if the tarball isn't present before runningdocker load.
19-19: Remove trailing spaces & blank lines.Clean up these lines to address YAML lint warnings (
trailing-spaces,empty-lines).Also applies to: 27-27
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/sanity.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-jest-unit-tests
- GitHub Check: build-noobaa-image
🔇 Additional comments (2)
.github/workflows/unit.yaml (2)
14-18: Verify artifact name and structure.Ensure the upstream
build-noobaajob uploads an artifact namednoobaa-testercontaining the.tarat the root so thatactions/download-artifactcorrectly restores/tmp/noobaa-tester.tar.
25-26: Test commands updated correctly.Adding
-o testermatches the new image loading logic and appears correct.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
de82d75 to
25e0998
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/unit.yaml (3)
14-18: Download and prepare the NooBaa tester artifact.Downloading the
noobaa-testerartifact and placing it in/tmpallows subsequent steps to load the Docker image. Confirm the artifact name matches the producer workflow and consider using the runner’s temp directory ($RUNNER_TEMP) instead of hardcoding/tmpfor better portability.Would you like a sample update using
$RUNNER_TEMPfor artifact download path?
19-19: Remove trailing spaces.Static analysis flagged trailing whitespace on this line; please remove it to avoid lint errors.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
27-27: Remove extra blank line.Static analysis warns of an unnecessary blank line here; removing it will keep the file tidy.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/sanity.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-jest-unit-tests
- GitHub Check: build-noobaa-image
🔇 Additional comments (4)
.github/workflows/unit.yaml (4)
2-2: Enable reusable workflow trigger.Switching to
on: [workflow_call]makes this workflow reusable only viaworkflow_call, which aligns with the centralization objective. Ensure no unintended triggers are needed.
6-6: Explicit job naming improves clarity.The job is now explicitly named
run-unit-tests, making outputs more readable and logs easier to locate.
20-21: Load the Docker image.The
docker loadcommand correctly imports the tester image for use in this job. Consider adding anidto this step if later steps need to reference its output.
25-26: Include tester option in test commands.Appending
-o testerto bothmake testandmake root-perm-testensures the correct image is used. Confirm that both commands exit on failure (set -eis default), so the step fails appropriately if tests fail.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
25e0998 to
ccefb3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/unit.yaml (3)
14-18: Consider using GitHub Container Registry instead of tar artifacts
Pulling and extracting a tarball adds latency. As noted in the PR objectives, you could push these images to GHCR anddocker pullthem directly in your workflows to speed up CI and reduce storage overhead.
19-19: Remove trailing whitespace
Line 19 contains only spaces. Please delete trailing spaces to satisfy YAML lint rules and avoid churn in diffs.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
27-27: Eliminate extra blank line
YAML lint warns about too many consecutive blank lines. Please remove the redundant empty line at 27.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/sanity.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[warning] 9-9: too many spaces after colon
(colons)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-noobaa-image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (3)
.github/workflows/unit.yaml (3)
2-2: Verifyworkflow_call-only trigger
You’ve removedpush/pull_requesttriggers in favor ofon: [workflow_call]. Ensure this workflow is always invoked by your orchestrator (run-pr-tests.yaml) or add alternative triggers (e.g.,workflow_dispatch) if you still need to run tests directly.
20-21: Loading Docker image looks correct
Thedocker load --input /tmp/noobaa-tester.tarstep properly restores the tester image for downstream steps.
25-26: Updated test commands are good
Including-o testeron bothmake testandmake root-perm-testaligns with the new image-loading approach.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
ccefb3a to
d9666c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/unit.yaml (3)
21-22: Consider using container registry instead of TAR artifacts
Loading images from tar introduces overhead. You could explore using GitHub Container Registry (GHCR) to push/pull Docker images directly and speed up CI.
20-20: Remove trailing spaces
Line 20 contains trailing spaces; remove them to satisfy YAML lint rules.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: trailing spaces
(trailing-spaces)
28-28: Remove extra blank line
There's an extra blank line at the end of the multi-line script. Removing it will satisfy the empty-lines lint rule.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/sanity.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
🔇 Additional comments (4)
.github/workflows/unit.yaml (4)
2-2: Ensure correct invocation with workflow_call
Switching toworkflow_callmeans this workflow no longer runs on push/PR. Verify that the orchestrator workflow (run-pr-tests.yaml) correctly calls this action with the required inputs.
8-10: Principle of least privilege applied to permissions
Grantingactions: readandcontents: readscopes down access appropriately and satisfies checkout and artifact download requirements.
15-19: Artifact download step is correctly configured
Theactions/download-artifactstep pulls thenoobaa-testerimage tar from/tmp. Ensure the artifact name matches exactly what the build workflow produces.
26-27: Test commands updated with-o tester
Including the-o testerflag aligns with the loaded tester image. Confirm that all downstream test workflows have similar updates for consistency.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
d9666c0 to
1346009
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/unit.yaml (2)
15-19: Validate artifact naming and consider a registry-based approach.
Ensure the artifact namenoobaa-testerexactly matches what the build-noobaa workflow publishes to avoid download failures.
As an optimization, consider pushing the Docker image to GitHub Container Registry (ghcr.io/.../noobaa-tester:latest) and usingdocker pullhere to eliminate tar export/import overhead.
20-20: Remove trailing whitespace and extra blank lines.
Clean up the blank lines and trailing spaces to satisfy YAML lint rules.- (line 20) <trailing spaces> - (line 23) - (line 28) +<remove these blank lines>Also applies to: 23-23, 28-28
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/sanity-ssl.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
🔇 Additional comments (4)
.github/workflows/unit.yaml (4)
2-2: Workflow trigger updated toworkflow_call.
This aligns with the reusable workflow pattern and prevents redundant CI runs on direct pushes/pull requests.
8-10: Permissions scoped to least privilege.
Grantingactions: readandcontents: readprovides only the necessary access for artifact download and checkout.
21-22: Docker image loading step is correct.
Thedocker load --input /tmp/noobaa-tester.tarcommand properly restores the tester image for subsequent test commands.
26-27: Test commands updated for preloaded image.
Including-o testerensures tests run against the downloaded tester image as intended.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
2 times, most recently
from
a6d4e81 to
86636a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/unit.yaml (2)
20-20: Remove trailing whitespace
Line 20 contains trailing spaces, which may trigger lint warnings. Consider deleting the extra spaces.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: trailing spaces
(trailing-spaces)
28-28: Remove extra blank line
Avoid excessive blank lines to adhere to YAMLlint's empty-lines rule. Consider deleting this line.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/sanity.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-noobaa-image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (5)
.github/workflows/unit.yaml (5)
2-2: Workflow trigger switched toworkflow_call
This aligns the workflow for reuse in composite actions.
8-10: Permissions scoped for artifact download and checkout
Grantingactions: readandcontents: readprovides least privilege needed for downloading artifacts and checking out code.
15-19: Added step to download the NooBaa tester image artifact
Usingactions/download-artifactto fetch thenoobaa-testertarball is correct for loading the Docker image later.
21-23: Load the tester Docker image
Thedocker loadcommand correctly imports the image for subsequent test steps.
26-27: Include-o testeroption in test commands
Passing thetesterimage to bothmake testandmake root-perm-testensures tests run inside the correct service context.
Signed-off-by: nadav mizrahi <nadav.mizrahi16@gmail.com>
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
86636a3 to
11dc341
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/unit.yaml (3)
15-19: Use runner-provided temp directory for downloads
Hardcoding/tmpmay not be portable on non-Linux runners. Prefer the${{ runner.temp }}environment variable for temporary storage:- with: - name: noobaa-tester - path: /tmp + with: + name: noobaa-tester + path: ${{ runner.temp }}
21-23: Consider image cleanup or registry caching
Repeated tar loads can consume runner disk and time. You could:
- Pull the image from GitHub Container Registry instead of artifacts.
- Add a cleanup step before or after to remove stale images (
docker rmi noobaa-tester).
20-20: Remove trailing whitespace and extra blank lines
YAMLlint flagged trailing spaces at line 20 and an extra blank line at line 28. Please remove them to satisfy lint rules.Also applies to: 28-28
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-package-lock-validation
- GitHub Check: build-noobaa-image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (3)
.github/workflows/unit.yaml (3)
2-2: Switch to reusable trigger via workflow_call
Changing triggers toworkflow_callaligns the workflow with the new orchestrator pattern. Ensure downstream workflows pass all required inputs when invoking this.
8-10: Least-privilege permissions scoped correctly
Grantingactions: readandcontents: readis sufficient for artifact download and checkout. No broadread-allscope—good adherence to the principle of least privilege.
26-27: Test commands updated for tester image
Appending-o testeraligns with loading the tester image. Verify that all relevantmaketargets support this flag.
3 participants
Describe the Problem
currently noobaa and tester image are built for each task separately. causing extra load on the CI resources
Explain the Changes
###Gaps
Testing Instructions:
Summary by CodeRabbit
New Features
Chores