CI | pull noobaa-base image instead building it #9099
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughMultiple GitHub Actions workflow jobs across various YAML files were updated to include explicit Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (10)
🪧 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
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
0a24473 to
ec26739
Compare
nadavMiz
force-pushed
the
ci-download-bas-image
branch
4 times, most recently
from
462ef7e to
d697f5f
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
🧹 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.
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.
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
26f0195 to
3f8e931
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/run-pr-tests.yaml (1)
124-124: Remove trailing space in GitHub output echo.
An extra space after the redirect can introduce unintended whitespace in the output.Apply this diff:
- 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/ceph-nsfs-s3-tests.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/sanity.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/unit.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/warp-nc-tests.yaml
🧰 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: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: Summary
🔇 Additional comments (9)
.github/workflows/run-pr-tests.yaml (9)
45-45: Explicit job naming improves workflow readability.
Adding a descriptivenamefor thebuild-noobaa-imagejob makes the CI UI more understandable.
52-63: Tag preparation logic is solid.
Deriving branch-based Docker tags and exporting them as outputs sets up clear downstream pulling and building.
71-74: Leveraging Changed-Files action for precise rebuild detection.
Usingtj-actions/changed-files@v44correctly enumerates modified files, enabling conditional base-image rebuilds.
78-86: Efficient base-image rebuild detection.
Looping through${{ steps.changed_files.outputs.all_changed_files }}with exact-match grep flags only critical file changes.
89-106: Robust pull retry loop for base image.
Iterating date-tagged pulls up topull_triesdays maximizes image reuse before falling back to build.
126-134: Correct conditional flag usage.
Using-o builderto skip the builder phase when it's already pulled optimizes build time as intended.
135-136: Makefile dependencies leveraged correctly.
make tester -o baseproperly builds both primary and tester images via existing makefile rules.
143-148: Artifact upload is well configured.
Saving and uploadingnoobaa.tarensures the built image is available for downstream debugging.
150-155: Separate tester image artifact for clarity.
Uploadingnoobaa-tester.tarunder its own name aids in artifact management.
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
3f8e931 to
cb73044
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/run-pr-tests.yaml (2)
124-124: Remove trailing whitespace
The echo to$GITHUB_OUTPUTat the end of the builder pull step has a trailing space, which can cause malformed output.
146-146: Fix indentation underwith
Ensure the keys (name,path,retention-days) under thewith:block are indented two spaces further to satisfy YAML syntax.
📜 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/sanity.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-tests.yaml
🧰 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-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: Summary
🔇 Additional comments (9)
.github/workflows/run-pr-tests.yaml (9)
45-45: Named job improves readability
Adding an explicitnamefor thebuild-noobaa-imagejob clarifies its purpose in the workflow UI.
52-70: Tag preparation logic is solid
Usinggithub.base_reffor PRs versusgithub.ref_namefor pushes and echoingbasetags,buildertags, andpull_triesis correct and clear.
78-88: Efficient base-image change detection
The array declaration and exact-match grep ensure only critical files trigger a rebuild.
89-106: Robust pull loop for the base image
Switching toseqfor retries, proper logging, and tagging logic is effective.
107-124: Builder pull logic is sound
Conditions correctly handle when to pull the builder image from Quay before deciding on a local build.
126-134: Skip builder phase when image is pulled
The-o builderflag is correctly applied only when the builder image pull succeeds, leveraging make’s-obehavior to skip targets.
135-136: Leverage makefile dependencies
Usingmake tester -o baseappropriately builds both the noobaa and tester images due to the existing target relationship.
143-149: Artifact upload for noobaa image
The step correctly usesactions/upload-artifact@v4to persist thenoobaa.tarimage for downstream jobs.
150-156: Artifact upload for tester image
The step correctly persists thenoobaa-tester.tarartifact usingactions/upload-artifact@v4.
|
@nadavMiz Any name change will need a change in the required test in the checks @nimrod-becker FYI |
Signed-off-by: nadav mizrahi <[email protected]>
nadavMiz
force-pushed
the
ci-download-bas-image
branch
from
cb73044 to
b94a690
Compare
3 participants
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
Summary by CodeRabbit