CI | pull noobaa-base image instead building it #9099
Conversation
|
""" WalkthroughMultiple GitHub Actions workflow jobs across various YAML files were updated to include explicit Changes
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
nadavMiz
force-pushed
the
ci-download-bas-image
branch
8 times, most recently
from
4b6e6e2 to
5a4146c
Compare
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
5a4146c to
275fd53
Compare
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
275fd53 to
699987e
Compare
nadavMiz
force-pushed
the
ci-download-bas-image
branch
12 times, most recently
from
3b4ee11 to
58b3855
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: 1
🧹 Nitpick comments (5)
.github/workflows/run-pr-tests.yaml (5)
52-63: Parameterize the branch name
Hard-codingBRANCH=masterprevents using feature branches or future default-branch changes. Consider injecting the branch via an input or usingGITHUB_REF_NAME.- BRANCH=master + BRANCH=${GITHUB_REF_NAME:-master}
65-68: Pin GitHub Action versions
Usingtj-actions/changed-files@v44without a commit SHA can introduce breaking changes. For reproducibility, pin to a full commit.
83-100: Quote variable expansions and DRY up pull logic
Unquoted expansions can misbehave on filenames with spaces (unlikely here) and the pull loop is duplicated for base/builder. Consider extracting the pull-legacy loop into a reusable script section and quoting tags.-for i in {0..${{steps.prep.outputs.pull_tries}}} -... - docker pull ${base_tag} || continue +for i in {0..${{ steps.prep.outputs.pull_tries }}}; do + # reuse pull logic + tag="quay.io/${{ steps.prep.outputs.basetags }}${date}" + echo "$tag" + if docker pull "$tag"; then + docker tag "$tag" noobaa-base + output=true + break + fi done
101-118: DRY the builder pull step and quote tags
The builder pull loop mirrors the base logic. Extract it into a function or step template to reduce maintenance overhead, and quote expansions for safety.
119-127: Consistent step naming and flag handling
The step "Build noobaa-base image" uses lower-case flags inline. For consistency, consider movingflagscalculation to an earlierenvor dedicatedrunblock and align casing with other step names.
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 117-117: trailing spaces
(trailing-spaces)
[warning] 123-123: wrong indentation: expected 14 but found 10
(indentation)
[warning] 139-139: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: Summary
🔇 Additional comments (12)
.github/workflows/nc_unit.yml (1)
6-6: Explicit job name improves UI clarity.Adding
name: Non Containerized Unit Teststo therun-nc-unit-testsjob makes the workflow more readable in GitHub Actions..github/workflows/ceph-nsfs-s3-tests.yaml (1)
6-6: Consistent job naming enhances readability.Introducing
name: NSFS Ceph S3 Testsat the job level aligns with other workflows and clarifies the job’s purpose in the Actions UI..github/workflows/unit.yaml (1)
6-6: Add job-level display name for clarity.The
name: Unit Testsfield on therun-unit-testsjob makes the workflow UI easier to navigate..github/workflows/postgres-unit-tests.yaml (1)
6-6: Explicit job name for better visibility.Adding
name: Unit Tests with Postgresto therun-unit-tests-postgresjob improves consistency and clarity..github/workflows/sanity.yaml (1)
6-6: Job-level name added for readability.The
name: Sanity Testsattribute on therun-sanity-testsjob aligns with the naming convention used across workflows..github/workflows/sanity-ssl.yaml (1)
6-6: Add explicit job name for clarity
Naming the job as "Sanity SSL Tests" improves readability in the GitHub Actions UI and logs..github/workflows/ceph-s3-tests.yaml (1)
6-6: Add explicit job name for clarity
Naming the job as "Ceph S3 Tests" improves readability in the GitHub Actions UI and logs..github/workflows/warp-nc-tests.yaml (1)
7-7: Add explicit job name for clarity
Naming the job as "Warp NC Tests" improves readability in the GitHub Actions UI and logs..github/workflows/warp-tests.yaml (1)
6-6: Add explicit job name for clarity
Naming the job as "Warp Tests" improves readability in the GitHub Actions UI and logs..github/workflows/run-pr-tests.yaml (3)
45-45: Explicitly name the build job
Addingname: Build Noobaa Imagebrings consistency with other workflows and makes the CI output self-documenting.
136-142: Upload base image artifact
Uploadingnoobaa-imageensures downstream jobs consume the correct image. The addition aligns with the new pull/build strategy.
143-149: Upload tester image artifact
Uploadingnoobaa-testercompletes the CI flow by preserving the tester image. This change is correct and necessary.
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
a813bc2 to
4354c0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/run-pr-tests.yaml (1)
119-127: Simplify flag assignment for base build
Relying on a literaltrue/falsecommand can be opaque. Consider an explicit conditional block or defaultingflagsto an empty string for clarity.flags="" if [ "${{ steps.should_build_builder.outputs.should_build }}" = "false" ]; then flags="-o builder" fi make base ${flags}
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/sanity-ssl.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/unit.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/postgres-unit-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 117-117: trailing spaces
(trailing-spaces)
[warning] 123-123: wrong indentation: expected 14 but found 10
(indentation)
[warning] 139-139: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: Summary
🔇 Additional comments (7)
.github/workflows/run-pr-tests.yaml (7)
45-45: Confirm explicit job name for clarity
Addingname: Build Noobaa Imageenhances job identification in the CI UI.
52-63: Prepare image tags correctly
The step correctly computes and exportsbasetags,buildertags, andpull_triesvia$GITHUB_OUTPUT.
65-67: Pintj-actions/changed-filesversion
Locking the changed-files action tov44ensures consistent output formatting.
69-82: Detect critical file changes efficiently
The bash array andgrep -xqcheck correctly flags modifications topackage.json,base.dockerfile, and.nvmrc.
128-129: Verify correctmaketarget in build step
The stepname: Build noobaa imageinvokesmake tester -o base. Please confirm this target and flag are intentional (e.g., should it bemake noobaaor another invocation?).
136-142: Approve artifact upload for NooBaa image
The upload-artifact step fornoobaa-imageis correctly configured with a 1-day retention.
143-149: Approve artifact upload for NooBaa-tester image
The upload-artifact step fornoobaa-testeris correctly configured.
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
4354c0c to
6d6b38a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/run-pr-tests.yaml (1)
101-118: Clean up trailing whitespace and output naming
- Remove the trailing space on line 117 to prevent
$GITHUB_OUTPUTparsing issues.- Consider renaming the builder step’s output key (e.g.
should_build_builder) to avoid confusion with the base image step’s output.
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 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/ceph-s3-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/unit.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/sanity.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 117-117: trailing spaces
(trailing-spaces)
[warning] 137-137: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: Summary
🔇 Additional comments (6)
.github/workflows/run-pr-tests.yaml (6)
45-45: Add explicit job name for clarity
Providing a human-readablenamefor thebuild-noobaa-imagejob improves CI readability.
65-68: Use changed-files action correctly
Thetj-actions/changed-files@v44step is configured properly to detect diffs. This will drive the conditional logic as expected.
69-82: Base image rebuild condition is sound
The loop over changed files and exact matching against thebase_filesarray correctly determines when to rebuild the base image. Good use ofgrep -x -qfor precise matching.
83-100: Retry logic for pulling base image looks solid
Looping withseqfor dates and breaking on the first successful pull/tag ensures robustness without failing the workflow prematurely.
134-140: Upload artifact steps are configured correctly
Explicit artifact names and retention policies are clear and aligned with best practices.
141-147: Tester image upload step looks good
Matching the pattern of the previous upload step ensures consistency in naming and retention.
nadavMiz
force-pushed
the
ci-download-bas-image
branch
3 times, most recently
from
7cf4bab to
b16166f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/run-pr-tests.yaml (2)
119-125: Fix in-shell conditional evaluation in run block.
Embedding${{ }}directly in a shell command underset -ewill cause the script to exit when the expression isfalse. Use a properifstatement instead.- ${{ steps.should_build_builder.outputs.should_build == 'false' }} && flags="-o builder" - make base ${flags} + if [ "${{ steps.should_build_builder.outputs.should_build }}" = "false" ]; then + flags="-o builder" + fi + make base ${flags}
101-118: Fix trailing space causing GITHUB_OUTPUT parsing errors.
The echo at line 117 includes a trailing space before the newline, which can break output parsing.Apply this diff:
- echo "should_build=${output}" >> $GITHUB_OUTPUT + echo "should_build=${output}" >> $GITHUB_OUTPUT
🧹 Nitpick comments (2)
.github/workflows/run-pr-tests.yaml (2)
69-82: Conditional base-image rebuild check looks solid.
Proper array syntax and exact-match grep ensure only root-level critical files trigger a base-image rebuild.Consider using
basename "$file"or${file##*/}if you ever move critical files into subdirectories to avoid false negatives.
83-100: Retry loop for pulling base image is well-structured.
Theseqloop with|| continuehandles pull failures gracefully, and tagging upon success is correct.Quote variable expansions (
"${base_tag}") indocker pullanddocker tagto avoid issues with unusual characters.
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/unit.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/warp-nc-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 117-117: trailing spaces
(trailing-spaces)
[warning] 137-137: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: Summary
🔇 Additional comments (5)
.github/workflows/run-pr-tests.yaml (5)
45-45: Approval: Job display name provided for build-noobaa-image.
Job name “Build Noobaa Image” improves readability in the workflow UI.
52-63: Prepare tags step logic is correct.
Dynamic generation of builder/base tags andpull_triesoutput is sound. Usinggithub.base_refensures branch-specific image pulls.
65-68: CI optimization: Changed-files action is correctly configured.
The use oftj-actions/changed-files@v44will accurately detect modified files for conditional builds.
134-139: Approve artifact upload step for noobaa image.
Naming and retention policy are clear and correctly declared.
141-146: Approve artifact upload step for noobaa-tester image.
Consistent with the previous upload step and uses recommended actions.
nadavMiz
force-pushed
the
ci-download-bas-image
branch
2 times, most recently
from
0a24473 to
ec26739
Compare
nadavMiz
force-pushed
the
ci-download-bas-image
branch
4 times, most recently
from
462ef7e to
d697f5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/run-pr-tests.yaml (2)
71-74: Pin external actions to immutable references
You’re usingtj-actions/changed-files@v44. To avoid unexpected breaking changes, consider pinning to a full commit SHA rather than a floating tag:uses: tj-actions/changed-files@<commit-sha>
107-124: Tidy up builder-pull step and id naming
- Remove the trailing space on line 123 to prevent whitespace in the output file.
- The step id
should_build_builderis misleading—it actually pulls the builder image. Consider renaming topull_builder_imageor similar for clarity.-echo "should_build=${output}" >> $GITHUB_OUTPUT +echo "should_build=${output}" >> $GITHUB_OUTPUT
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/unit.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 123-123: trailing spaces
(trailing-spaces)
[warning] 145-145: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: Summary
🔇 Additional comments (6)
.github/workflows/run-pr-tests.yaml (6)
45-45: Explicit job naming added
Thebuild-noobaa-imagejob now has a human-readablenameattribute, improving the workflow UI and logs.
75-87: Base-image rebuild detection is good
The array-based check for critical files is straightforward and correct. The outputshould_buildwill drive the subsequent pull/build logic as intended.
89-105: Graceful retry logic for pulling base image
The loop withseqand retry semantics is solid. Tagging the pulled image tonoobaa-baseand capturing success inpull_succeedfits the intended flow.
125-133: Verify multi-lineif:expression is parsed correctly
Splitting theif: ${{ … }}across two lines can confuse YAML parsing in GitHub Actions. Please confirm this condition behaves as intended, or collapse it into a single line or use a block scalar:if: ${{ steps.should_build_base.outputs.should_build == 'true' || steps.pull_base_image.outputs.pull_succeed == 'false' }}
142-147: Explicit artifact upload step for noobaa image
Adding a descriptivenameto this upload step improves readability when viewing artifacts.
149-154: Explicit artifact upload step for tester image
TheUpload noobaa-tester docker imagestep is now named clearly, making it easier to track in the workflow UI.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/run-pr-tests.yaml (2)
78-87: Confirm critical file list and iteration logic
Verify thatbase.dockerfile(case-sensitive) matches the actual filename. Also consider reading the changed-files output line-by-line to avoid whitespace splitting:while IFS= read -r file; do # ... done <<< "${{ steps.changed_files.outputs.all_changed_files }}"
109-124: Remove trailing space in GitHub output echo
The line:echo "should_build=${output}" >> $GITHUB_OUTPUThas an extra space at the end. Drop it to avoid adding unwanted whitespace:
- echo "should_build=${output}" >> $GITHUB_OUTPUT + echo "should_build=${output}" >> $GITHUB_OUTPUT
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/sanity-ssl.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/nc_unit.yml
- .github/workflows/unit.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 123-123: trailing spaces
(trailing-spaces)
[warning] 145-145: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: Summary
🔇 Additional comments (6)
.github/workflows/run-pr-tests.yaml (6)
45-45: Add a clear job name for readability
Thename: Build Noobaa Imagedeclaration makes the job’s purpose explicit in the UI and logs.
93-105: Pull retry loop is solid
This loop correctly retries pulling historical tags for the base image and tags the first successful pull asnoobaa-base.
125-133: Conditional flags logic is correct
Using theshould_build_builderflag to setflags="-o builder"only when a builder image was pulled ensuresmake base ${flags}behaves as intended.
135-135: Ensure correct target specification
Switching tomake tester -o basemakes the noobaa image build reference the prepared base image. This aligns with the designed workflow.
142-147: Explicit artifact upload step name
Naming the stepUpload noobaa docker imageclarifies which artifact is being published in the logs.
149-154: Explicit artifact upload step name
Naming the stepUpload noobaa-tester docker imageimproves readability in the CI output.
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
d697f5f to
9cb22ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/run-pr-tests.yaml (1)
134-136: Fix build commands for noobaa and tester.The step name suggests building both images but only runs the tester target. You need to build the main
noobaaimage as well.-run: make tester -o base +run: | + make noobaa -o base + make tester -o base
🧹 Nitpick comments (3)
.github/workflows/run-pr-tests.yaml (3)
45-49: Fix indentation to satisfy YAMLlint.YAMLlint flagged wrong indentation on lines 45 and 49. Align these lines to match the 2-space per level convention. For example:
- name: Build Noobaa Image + name: Build Noobaa Image - - name: Prepare Tags + - name: Prepare Tags
123-123: Remove trailing whitespace.YAMLlint reports a trailing space at the end of this line. Trim it to prevent formatting warnings.
145-145: Fix indentation to satisfy YAMLlint.This line should be indented two spaces deeper to match the surrounding
uses:andwith:blocks:- name: noobaa-image + name: noobaa-image
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/sanity-ssl.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/nc_unit.yml
- .github/workflows/unit.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/run-pr-tests.yaml (1)
Learnt from: nadavMiz
PR: noobaa/noobaa-core#9099
File: .github/workflows/run-pr-tests.yaml:52-70
Timestamp: 2025-06-19T10:01:46.926Z
Learning: In the noobaa-core repository, branch names don't contain forward slashes, so Docker tag sanitization is not needed in GitHub Actions workflows. Manual full builds and automated builds should use consistent tag naming to avoid mismatches.
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 123-123: trailing spaces
(trailing-spaces)
[warning] 145-145: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
- GitHub Check: Summary
🔇 Additional comments (8)
.github/workflows/run-pr-tests.yaml (8)
45-45: Explicit job naming improves readability.Adding
name: Build Noobaa Imagemakes the workflow UI clearer and helps distinguish this job from others in the Actions view.
52-70: Prepare Tags step logic is solid.The dynamic computation of
BRANCH,BUILDER_TAGS,BASE_TAGS, andpull_triesis clear and correct. Emitting them via$GITHUB_OUTPUTfor downstream consumption is the right approach.
71-88: Changed-files detection for base rebuild is correct.The array declaration and loop correctly identify when critical files change. Using
printf … | grep -xensures exact filename matches.
89-105: Pull logic for noobaa-base image is robust.Retrying pulls over a date range and tagging on success handles registry hiccups gracefully.
107-122: Pull logic for noobaa-builder image looks good.The conditional and retry loop correctly decide when to fall back to building locally.
125-133: Build noobaa-base step conditional is accurate.Using a shell
ifto toggle the-o builderflag based on the previous pull output is handled correctly, andmake base ${flags}will run as intended.
142-147: Upload noobaa docker image step is sound.Saving and uploading the
noobaaimage as an artifact with a 1-day retention aligns with the CI objective.
149-154: Upload noobaa-tester docker image step is correct.The artifact upload for the tester image, including
retention-days, is properly configured.
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
9cb22ed to
071ab43
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/run-pr-tests.yaml (1)
134-136: Incorrect build command for Noobaa images
The step namedBuild noobaa and tester imagesonly runsmake tester -o base, skipping the primary Noobaa image build. This will result in missing the main image artifact.Apply this diff to split into two steps:
- - name: Build noobaa and tester images - run: make tester -o base + - name: Build Noobaa image + run: make noobaa -o base + + - name: Build noobaa-tester image + run: make tester -o base
🧹 Nitpick comments (3)
.github/workflows/run-pr-tests.yaml (3)
52-70: Quote and export environment variables
In thePrepare Tagsstep, wrap assignments in quotes and export them to prevent word-splitting. Also quote the$GITHUB_OUTPUTredirection to handle any path quirks.Apply this diff inside the
runblock:- DOCKER_BUILDER_IMAGE=noobaa/noobaa-builder - DOCKER_BASE_IMAGE=noobaa/noobaa-base + export DOCKER_BUILDER_IMAGE="noobaa/noobaa-builder" + export DOCKER_BASE_IMAGE="noobaa/noobaa-base" ... - echo "basetags=${BASE_TAGS}" >> $GITHUB_OUTPUT + echo "basetags=${BASE_TAGS}" >> "$GITHUB_OUTPUT" - echo "buildertags=${BUILDER_TAGS}" >> $GITHUB_OUTPUT + echo "buildertags=${BUILDER_TAGS}" >> "$GITHUB_OUTPUT" - echo "pull_tries=${EARLIEST_VERSION_PAST}" >> $GITHUB_OUTPUT + echo "pull_tries=${EARLIEST_VERSION_PAST}" >> "$GITHUB_OUTPUT"
61-61: Remove trailing spaces in shell steps
Trailing whitespace in the comment on line 61 and theechoon line 123 causes lint errors. Remove the extra spaces at the end of these lines.Also applies to: 123-123
128-133: Initializeflagsbefore conditional usage
If the builder pull succeeds,$flagsmay be undefined when passed tomake base. Defineflags=""before theifto prevent unbound variable warnings.-run: | - if [ "${{ steps.should_build_builder.outputs.should_build }}" = 'false' ]; then - flags="-o builder" - fi - make base ${flags} +run: | + flags="" + if [ "${{ steps.should_build_builder.outputs.should_build }}" = 'false' ]; then + flags="-o builder" + fi + make base ${flags}
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 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/unit.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/run-pr-tests.yaml (1)
Learnt from: nadavMiz
PR: noobaa/noobaa-core#9099
File: .github/workflows/run-pr-tests.yaml:52-70
Timestamp: 2025-06-19T10:01:46.926Z
Learning: In the noobaa-core repository, branch names don't contain forward slashes, so Docker tag sanitization is not needed in GitHub Actions workflows. Manual full builds and automated builds should use consistent tag naming to avoid mismatches.
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 61-61: trailing spaces
(trailing-spaces)
[error] 123-123: trailing spaces
(trailing-spaces)
[warning] 145-145: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: Summary
🔇 Additional comments (3)
.github/workflows/run-pr-tests.yaml (3)
75-88: The array declaration and loop inShould build noobaa base imagewere already corrected and match the expected behavior.
89-106: Pull base image logic looks sound
The retry loop correctly attempts to pull thenoobaa-baseimage across dates and tags it locally upon success.
107-124: Pull builder image logic is valid
The conditional retry over the builder tags and the output flagshould_buildcorrectly determine whether to fall back to a local build.
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
071ab43 to
0cf2663
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/run-pr-tests.yaml (2)
143-148: Consistent step naming.
Capitalize “Docker” in the step name to match other job labels:- - name: Upload noobaa docker image + - name: Upload Noobaa Docker image
150-155: Consistent step naming.
Align with naming conventions by capitalizing and formatting:- - name: Upload noobaa-tester docker image + - name: Upload Noobaa-Tester Docker image
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/unit.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/sanity-ssl.yaml
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/run-pr-tests.yaml (1)
Learnt from: nadavMiz
PR: noobaa/noobaa-core#9099
File: .github/workflows/run-pr-tests.yaml:52-70
Timestamp: 2025-06-19T10:01:46.926Z
Learning: In the noobaa-core repository, branch names don't contain forward slashes, so Docker tag sanitization is not needed in GitHub Actions workflows. Manual full builds and automated builds should use consistent tag naming to avoid mismatches.
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 124-124: trailing spaces
(trailing-spaces)
[warning] 146-146: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
- GitHub Check: Summary
🔇 Additional comments (4)
.github/workflows/run-pr-tests.yaml (4)
45-45: Explicit job name improves readability.
Addingname: Build Noobaa Imagealigns with other workflows and makes the job easier to identify in the UI.
52-70: Tag preparation logic is sound.
This block correctly derives builder and base image tags from the branch context and exports them viaGITHUB_OUTPUT.
71-73: Verify changed-files action version.
Ensure thattj-actions/changed-files@v44exists and is supported; consider pinning to a major version or digest to avoid unexpected breaks if a new release is published.
75-87: Selective base rebuild on critical files.
The loop over changed files correctly triggers a base-image rebuild only whenpackage.json,base.dockerfile, or.nvmrcare modified.
Signed-off-by: nadav mizrahi <[email protected]>
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
0cf2663 to
26f0195
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/run-pr-tests.yaml (3)
107-124: Remove trailing space in output echo
The final echo appends a trailing space, which can inject whitespace into theshould_buildoutput and break subsequentifconditions.Apply this diff:
- echo "should_build=${output}" >> $GITHUB_OUTPUT + echo "should_build=${output}" >> $GITHUB_OUTPUT
143-148: Fix indentation in upload step
YAMLlint flags inconsistent indentation in theUpload noobaa docker imageblock. Align each level by 2 spaces understeps:. For example:- - name: Upload noobaa docker image - uses: actions/upload-artifact@v4 - with: - name: noobaa-image - path: noobaa.tar - retention-days: "1" + - name: Upload noobaa docker image + uses: actions/upload-artifact@v4 + with: + name: noobaa-image + path: noobaa.tar + retention-days: "1"
150-155: Align indentation for tester upload
Similarly, correct the indentation for theUpload noobaa-tester docker imagestep:- - name: Upload noobaa-tester docker image - uses: actions/upload-artifact@v4 - with: - name: noobaa-tester - path: noobaa-tester.tar - retention-days: "1" + - name: Upload noobaa-tester docker image + uses: actions/upload-artifact@v4 + with: + name: noobaa-tester + path: noobaa-tester.tar + retention-days: "1"
📜 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(1 hunks).github/workflows/warp-tests.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/unit.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/nc_unit.yml
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/run-pr-tests.yaml (3)
Learnt from: nadavMiz
PR: noobaa/noobaa-core#9099
File: .github/workflows/run-pr-tests.yaml:135-136
Timestamp: 2025-06-19T11:03:49.906Z
Learning: In the noobaa-core project, the `make tester` command also calls `make noobaa` due to makefile target dependencies, so running `make tester` will build both the noobaa and tester images.
Learnt from: nadavMiz
PR: noobaa/noobaa-core#9099
File: .github/workflows/run-pr-tests.yaml:52-70
Timestamp: 2025-06-19T10:01:46.926Z
Learning: In the noobaa-core repository, branch names don't contain forward slashes, so Docker tag sanitization is not needed in GitHub Actions workflows. Manual full builds and automated builds should use consistent tag naming to avoid mismatches.
Learnt from: nadavMiz
PR: noobaa/noobaa-core#9099
File: .github/workflows/run-pr-tests.yaml:126-134
Timestamp: 2025-06-19T11:04:50.148Z
Learning: In make commands, the `-o` flag followed by a target name (e.g., `-o builder`) tells make to skip building that specific target, assuming it's already up to date. This is used in the noobaa-core project's workflow to skip building the builder image when it was successfully pulled from a registry.
🪛 YAMLlint (1.37.1)
.github/workflows/run-pr-tests.yaml
[warning] 45-45: wrong indentation: expected 8 but found 4
(indentation)
[warning] 49-49: wrong indentation: expected 10 but found 6
(indentation)
[error] 124-124: trailing spaces
(trailing-spaces)
[warning] 146-146: wrong indentation: expected 14 but found 10
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
- GitHub Check: Summary
🔇 Additional comments (6)
.github/workflows/run-pr-tests.yaml (6)
45-45: Add explicit job name for clarity
Naming thebuild-noobaa-imagejob as "Build Noobaa Image" improves readability in the Actions UI.
52-63: Dynamic tag preparation for pull vs push
ThePrepare Tagsstep correctly distinguishes pull requests (github.base_ref) from push events (github.ref_name) and emitsbuildertags,basetags, andpull_triesvia$GITHUB_OUTPUT. This enables downstream steps to use consistent Docker tag prefixes.Also applies to: 67-70
78-86: Detect base rebuild on critical file changes
Theshould_build_baselogic uses a proper Bash array and exact-match grep to trigger a base-image rebuild only whenpackage.json,base.dockerfile, or.nvmrcchange.
89-99: Pull historical base images with retries
The loop overseq 0 ${{ steps.prep.outputs.pull_tries }}and date-based tags is a robust way to fetch recent images. Breaking on first success and tagging locally (noobaa-base) is implemented correctly.
126-134: Conditional base-image build with skip-flag
Themake base ${flags}invocation correctly uses-o builderto skip rebuilding the builder stage when the builder image was pulled, otherwise building both stages as needed.
135-136: Leverage Makefile dependencies for combined build
Runningmake tester -o baseis sufficient to build both the primarynoobaaand tester images, thanks to the Makefile’s target dependencies.
Describe the Problem
Currently, we build the noobaa-base and noobaa-builder images for every commit and pull request. This significantly increases CI workflow time.
Explain the Changes
We now pull the base image from a Quay repository instead of building it on every run — unless the PR includes changes to files that require a new build:
new build noobaa image flow:
a. Pull the
noobaa-baseimage from Quaya. Pull the
noobaa-builderimage from Quay.b. Build a new
noobaa-baseimage from the builder image.c. If pulling the
builder imagealso fails, build it locally.Additional Updates
Improved the display names of test jobs for better readability in the CI output.
Issues: Fixed #xxx / Gap #xxx
GAPS
Testing Instructions:
in all cases build should succeed, and test worfklows should run
Summary by CodeRabbit