feat: Add support for building execution image for ubuntu-noble. #918
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe updates introduce support for building and using a new Ubuntu Noble-based execution image, including a new Dockerfile, build scripts, and workflow automation. Additionally, MariaDB Python dependency versions are updated, and a type annotation is corrected. Workflow parallelism is now dynamically set based on available processors. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant GitHub Actions Runner
participant Docker
participant Repo
Workflow->>GitHub Actions Runner: Detect changes (filter-relevant-changes)
GitHub Actions Runner->>Workflow: Output ubuntu_noble_image_changed
alt If ubuntu_noble_image_changed
Workflow->>GitHub Actions Runner: Run ubuntu-noble-execution-image job
GitHub Actions Runner->>Repo: Checkout code and submodules
GitHub Actions Runner->>Docker: Build Ubuntu Noble image (build.sh)
Docker->>Docker: Use Dockerfile and setup-scripts
Docker->>Docker: Install prebuilt packages (install-prebuilt-packages.sh)
Docker->>GitHub Actions Runner: Tag and label image
end
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
components/job-orchestration/pyproject.toml (1)
17-17: Verify version constraint intent
Same recommendation as above: ensure that the bump to~1.1.10aligns with your minimum version requirement and does not inadvertently exclude earlier 1.1.x patches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/clp-execution-image-build.yaml(1 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(1 hunks)components/clp-py-utils/clp_py_utils/sql_adapter.py(1 hunks)components/clp-py-utils/pyproject.toml(1 hunks)components/job-orchestration/pyproject.toml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/clp-execution-image-build.yaml
74-74: property "ubuntu_noble_image_changed" is not defined in object type {ubuntu_jammy_image_changed: string}
(expression)
🔇 Additional comments (2)
components/clp-py-utils/pyproject.toml (1)
16-16: Verify version constraint intent
Please confirm that using~1.1.10(i.e. >=1.1.10,<1.2.0) was intentional and does not exclude critical patch releases between 1.1.0 and 1.1.9. If the goal is to support any 1.1.x release, consider switching to~1.1.0.components/clp-py-utils/clp_py_utils/clp_config.py (1)
750-750: Correctly commented out redundant codeThis line is now redundant since the prefix is conditionally added in the if-else block above, so it's appropriate to comment it out.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
tools/docker-images/clp-execution-base-ubuntu-noble/Dockerfile(1 hunks)tools/docker-images/clp-execution-base-ubuntu-noble/build.sh(1 hunks)tools/docker-images/clp-execution-base-ubuntu-noble/setup-scripts/install-prebuilt-packages.sh(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
tools/docker-images/clp-execution-base-ubuntu-noble/Dockerfile
[LOW] 6-6: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 1-16: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-16: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
tools/docker-images/clp-execution-base-ubuntu-noble/Dockerfile
[error] 6-6: Use COPY instead of ADD for files and folders
(DL3020)
🪛 Shellcheck (0.10.0)
tools/docker-images/clp-execution-base-ubuntu-noble/build.sh
[warning] 25-25: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
tools/docker-images/clp-execution-base-ubuntu-noble/setup-scripts/install-prebuilt-packages.sh
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
736-745:⚠️ Potential issueAdd registry prefix to Ubuntu Noble execution container
The Noble branch currently omits the
ghcr.io/y-scope/clp/prefix, causing runs to look for a local or Docker Hub image instead of the intended GHCR registry. For consistency with other releases, prefix the dev-tagged Noble image as well.Please update with this fix:
if "noble" == os_release["VERSION_CODENAME"]: self.execution_container = ( f"clp-execution-x86-{os_release['ID']}-{os_release['VERSION_CODENAME']}:dev" ) + self.execution_container = "ghcr.io/y-scope/clp/" + self.execution_container else: self.execution_container = ( f"clp-execution-x86-{os_release['ID']}-{os_release['VERSION_CODENAME']}:main" ) self.execution_container = "ghcr.io/y-scope/clp/" + self.execution_container
There was a problem hiding this comment.
I should've realized this earlier, but technically, for complete support for Ubuntu Noble, we should add a container like clp-core-dependencies-x86-ubuntu-noble. There are also some other related changes we need to make---you could take a look at the PR where we dropped support for Ubuntu Focal. Although, note that in that PR, we changed the OS that the package uses from Focal to Jammy---whereas in this PR, we don't want to change the OS the package uses from Jammy to Noble.
Can you also address the rabbit's feedback?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/clp-execution-image-build.yaml(3 hunks)
🔇 Additional comments (2)
.github/workflows/clp-execution-image-build.yaml (2)
28-30: Filter output for Noble support added
You’ve correctly introduced theubuntu_noble_image_changedoutput in thefilter-relevant-changesjob, mapping it tosteps.filter.outputs.ubuntu_noble_image. This addresses the previous gap and ensures the Noble-specific job can conditionally run.
46-53: Paths-filter updated for Ubuntu Noble
Thepaths-filterconfiguration now includes anubuntu_noble_imagefilter covering the newtools/docker-images/clp-execution-base-ubuntu-noble/**/*path (alongside the shared action and workflow YAML). This will correctly gate the Noble build job to only trigger on relevant file changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/docker-images/clp-execution-base-ubuntu-noble/Dockerfile (1)
14-16: 🧹 Nitpick (assertive)Enhance security with a non-root user and add HEALTHCHECK
It’s best practice to drop root privileges for runtime and add a health check for production readiness. You can integrate it like this:
# After cleaning caches RUN mkdir -p /home/clpuser \ + && groupadd --system clpuser && useradd --system --gid clpuser --home /home/clpuser clpuser \ + && chown -R clpuser:clpuser /home/clpuser +USER clpuser # Flatten the image FROM scratch COPY --from=base / / +# Basic health check (adjust as needed) +HEALTHCHECK --interval=30s --timeout=5s CMD ["true"]This setup creates an unprivileged
clpuserfor runtime and adds a simple health probe.🧰 Tools
🪛 Checkov (3.2.334)
[LOW] 1-16: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-16: Ensure that a user for the container has been created
(CKV_DOCKER_3)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tools/docker-images/clp-execution-base-ubuntu-noble/Dockerfile(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
tools/docker-images/clp-execution-base-ubuntu-noble/Dockerfile
[LOW] 1-16: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-16: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🔇 Additional comments (3)
tools/docker-images/clp-execution-base-ubuntu-noble/Dockerfile (3)
1-3: Base image and working directory are appropriateStarting from
ubuntu:nobleand settingWORKDIR /rootprovides a clear, isolated stage for installing dependencies.🧰 Tools
🪛 Checkov (3.2.334)
[LOW] 1-16: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-16: Ensure that a user for the container has been created
(CKV_DOCKER_3)
5-7: COPY usage follows best practicesUsing
COPYinstead ofADDfor the setup-scripts avoids unintended side-effects like automatic archive extraction, adhering to Dockerfile best practices.
10-12: Cache cleanup is effectiveThe
apt-get cleancombined with removing/var/lib/apt/lists/*,/tmp/*, and/var/tmp/*correctly minimises the final image footprint.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
tools/docker-images/clp-execution-base-ubuntu-noble/build.sh (3)
1-9: Robust shebang and error flags
You’ve correctly used#!/usr/bin/env bashand enabledset -e,set -u,set -o pipefail. This is solid for fail-fast behaviour. (If you’d like, you can merge these intoset -euo pipefail.)
24-29: 🧹 Nitpick (assertive)Suppress SC2206 for array assignment
ShellCheck flags splitting in thebuild_cmdarray, but here you intentionally want separate elements"docker"and"build". To silence the warning, add:+# shellcheck disable=SC2206 build_cmd=( docker build --tag clp-execution-${arch_name}-ubuntu-noble:dev "$repo_root" --file "${script_dir}/Dockerfile" )🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 26-26: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
31-37: 🧹 Nitpick (assertive)Use
repo_rootfor Git metadata
Currently, Git commands run in$script_dir, which works but ties you to the script’s path. For clarity and reliability, switch to$repo_root:-if command -v git >/dev/null && git -C "$script_dir" rev-parse --is-inside-work-tree >/dev/null ; +if command -v git >/dev/null && git -C "$repo_root" rev-parse --is-inside-work-tree >/dev/null ; then build_cmd+=( - --label "org.opencontainers.image.revision=$(git -C "$script_dir" rev-parse HEAD)" - --label "org.opencontainers.image.source=$(git -C "$script_dir" remote get-url origin)" + --label "org.opencontainers.image.revision=$(git -C "$repo_root" rev-parse HEAD)" + --label "org.opencontainers.image.source=$(git -C "$repo_root" remote get-url origin)" ) fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tools/docker-images/clp-execution-base-ubuntu-noble/build.sh(1 hunks)tools/docker-images/clp-execution-base-ubuntu-noble/setup-scripts/install-prebuilt-packages.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
tools/docker-images/clp-execution-base-ubuntu-noble/build.sh
[warning] 26-26: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
🔇 Additional comments (2)
tools/docker-images/clp-execution-base-ubuntu-noble/setup-scripts/install-prebuilt-packages.sh (1)
1-2: Shebang is portable
Using#!/usr/bin/env bashensures the script runs with Bash from the user’s PATH, which is ideal for cross-distribution portability.tools/docker-images/clp-execution-base-ubuntu-noble/build.sh (1)
13-22: Architecture detection is clear
Mappinguname -mvalues tox86andarm64covers the common cases and exits on unknown arches. This is straightforward and maintainable.
tools/docker-images/clp-execution-base-ubuntu-noble/setup-scripts/install-prebuilt-packages.sh
Show resolved
Hide resolved
tools/docker-images/clp-execution-base-ubuntu-noble/setup-scripts/install-prebuilt-packages.sh
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (3)
components/core/tools/scripts/lib_install/ubuntu-noble/install-prebuilt-packages.sh (1)
31-43: 🛠️ Refactor suggestionGuarantee
.debcleanup with atrap.If
dpkg --installfails, the temporary package remains. Immediately after creatingtask_pkg_path, add:trap 'rm -f "$task_pkg_path"' EXITThis ensures the file is removed on any exit. You may also echo progress steps for clarity.
components/core/tools/docker-images/clp-env-base-ubuntu-noble/Dockerfile (1)
1-17: 🧹 Nitpick (assertive)Enhance security and observability.
- Consider creating and switching to a non-root user to reduce attack surface.
- Add a
HEALTHCHECKinstruction to detect if the container is still functional at runtime.Example snippet:
# After flattening: USER clp HEALTHCHECK --interval=5m --timeout=3s \ CMD [ "pg_isready", "-q", "-h", "localhost", "-p", "5432" ]🧰 Tools
🪛 Checkov (3.2.334)
[LOW] 6-6: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 1-16: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-16: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
[error] 6-6: Use COPY instead of ADD for files and folders
(DL3020)
.github/workflows/clp-core-build.yaml (1)
56-58: 🧹 Nitpick (assertive)Quote owner string in
chownto avoid word splitting.ShellCheck warns on unquoted command substitution. Update as:
- run: "chown $(id -u):$(id -g) -R ." + run: "chown \"$(id -u):$(id -g)\" -R ."🧰 Tools
🪛 actionlint (1.7.7)
57-57: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
57-57: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/clp-core-build.yaml(6 hunks)components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile(1 hunks)components/core/tools/docker-images/clp-core-ubuntu-noble/build.sh(1 hunks)components/core/tools/docker-images/clp-env-base-ubuntu-noble/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-ubuntu-noble/build.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-noble/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-noble/install-packages-from-source.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-noble/install-prebuilt-packages.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/clp-core-build.yaml (1)
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.500Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
🪛 Checkov (3.2.334)
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
[LOW] 17-17: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 18-18: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 19-19: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 20-20: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 21-21: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 1-29: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-29: Ensure that a user for the container has been created
(CKV_DOCKER_3)
components/core/tools/docker-images/clp-env-base-ubuntu-noble/Dockerfile
[LOW] 6-6: Ensure that COPY is used instead of ADD in Dockerfiles
(CKV_DOCKER_4)
[LOW] 1-16: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-16: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
[warning] 5-5: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[info] 5-5: Avoid additional packages by specifying --no-install-recommends
(DL3015)
[error] 17-17: Use COPY instead of ADD for files and folders
(DL3020)
[error] 18-18: Use COPY instead of ADD for files and folders
(DL3020)
[error] 19-19: Use COPY instead of ADD for files and folders
(DL3020)
[error] 20-20: Use COPY instead of ADD for files and folders
(DL3020)
[error] 21-21: Use COPY instead of ADD for files and folders
(DL3020)
[warning] 29-29: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
components/core/tools/docker-images/clp-env-base-ubuntu-noble/Dockerfile
[error] 6-6: Use COPY instead of ADD for files and folders
(DL3020)
🪛 actionlint (1.7.7)
.github/workflows/clp-core-build.yaml
168-168: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
168-168: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
309-309: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
309-309: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
422-422: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
422-422: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
551-551: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
551-551: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
🪛 YAMLlint (1.37.1)
.github/workflows/clp-core-build.yaml
[error] 594-594: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (11)
components/core/tools/scripts/lib_install/ubuntu-noble/install-prebuilt-packages.sh (1)
9-29: 🧹 Nitpick (assertive)Reduce footprint with
--no-install-recommends.Include
--no-install-recommendsonapt-get installto avoid unnecessary dependencies:- DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ + DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \Likely an incorrect or invalid review comment.
components/core/tools/docker-images/clp-env-base-ubuntu-noble/build.sh (1)
13-19: Label handling is solid.The conditional Git-based labels are correctly quoted and applied only inside a work tree.
components/core/tools/docker-images/clp-core-ubuntu-noble/build.sh (1)
24-30: Label logic is correctly implemented.The conditional addition of Git labels is handled well and needs no changes.
components/core/tools/scripts/lib_install/ubuntu-noble/install-packages-from-source.sh (2)
9-10: Directory resolution logic is solid.Using
${BASH_SOURCE[0]}to computescript_dirand then referencing sibling scripts is correct and robust.
13-20: Installation sequence is clear and correct.The calls to each versioned install script are in the right order (Boost first), and error propagation is ensured by the strict shell flags.
.github/workflows/clp-core-build.yaml (6)
49-49: Outputs: Addedubuntu_noble_image_changed.The new output matches existing patterns for other Ubuntu variants and will drive conditional jobs correctly.
86-91: Filter: Included Ubuntu Noble paths.The
ubuntu_noble_imagefilter catches changes under both the generic scripts directory and theubuntu-noblesubdirectory, aligning with the other platform filters.
157-182: Job:ubuntu-noble-deps-imagefollows established pattern.The dependency image job mirrors
ubuntu-jammyandcentos-stream-9, with correctifcondition,needs, andwithparameters.🧰 Tools
🪛 actionlint (1.7.7)
168-168: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
168-168: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
283-343: Job:ubuntu-noble-binariesis consistent.Matrix setup, conditional artifact upload, and use of the published image logic align perfectly with existing variant jobs.
🧰 Tools
🪛 actionlint (1.7.7)
309-309: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
309-309: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
407-469: Job:ubuntu-noble-binaries-imagemirrors the Jammy workflow.Checkout, artifact download, metadata generation, and conditional push are all correctly parameterized for
ubuntu-noble.🧰 Tools
🪛 actionlint (1.7.7)
422-422: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
422-422: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
533-594: Job:ubuntu-noble-lintis in line withubuntu-jammy-lint.Cache keys, restore/save logic, and
run_commanduse the same structure, ensuring consistency across variants.🧰 Tools
🪛 actionlint (1.7.7)
551-551: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
551-551: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
🪛 YAMLlint (1.37.1)
[error] 594-594: no new line character at the end of file
(new-line-at-end-of-file)
components/core/tools/scripts/lib_install/ubuntu-noble/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-core-ubuntu-noble/build.sh
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-core-ubuntu-noble/build.sh
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-core-ubuntu-noble/build.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/ubuntu-noble/install-packages-from-source.sh
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-env-base-ubuntu-noble/Dockerfile
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
components/core/tools/scripts/lib_install/ubuntu-noble/install-all.sh (1)
18-20: 🧹 Nitpick (assertive)Add logging for CMake version check.
For consistency and traceability, echo before the final step:
echo "Verifying CMake version…" "${script_dir}/../check-cmake-version.sh"
♻️ Duplicate comments (5)
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile (3)
15-16: 🧹 Nitpick (assertive)Run as a non-root user for improved security.
Create and switch to a non-root user after copying files:
WORKDIR /clp +RUN groupadd -r clp && useradd -r -g clp clpuser && chown -R clpuser:clp /clp +USER clpuser
12-13: 🧹 Nitpick (assertive)Merge cleanup into the install step to flatten layers.
Combine the cache-cleanup with the install in a single
RUNto reduce image layers:-RUN apt-get clean \ - && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* + && apt-get clean \ + && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*(This snippet merges into the prior
RUN—see previous comment.)
5-9: 🛠️ Refactor suggestionPin package versions and avoid unnecessary recommends.
For reproducible, smaller images, specify exact package versions and use
--no-install-recommends. Example diff:-RUN apt-get update \ - && apt-get install -y \ - libcurl4 \ - libmariadb-dev \ - libssl-dev +RUN apt-get update \ + && DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ + libcurl4=<version> \ + libmariadb-dev=<version> \ + libssl-dev=<version> \ + && apt-get clean \ + && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*🧰 Tools
🪛 Hadolint (2.12.0)
[warning] 5-5: Pin versions in apt get install. Instead of
apt-get install <package>useapt-get install <package>=<version>(DL3008)
[info] 5-5: Avoid additional packages by specifying
--no-install-recommends(DL3015)
components/core/tools/scripts/lib_install/ubuntu-noble/install-prebuilt-packages.sh (1)
4-8: Duplicate: strict error handling flags applied
set -o pipefailis now present, matching other scripts and improving failure detection in pipelines..github/workflows/clp-core-build.yaml (1)
595-595: Add trailing newline at end-of-file
A final newline is required by some YAML parsers to avoid lint errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/clp-core-build.yaml(6 hunks)components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile(1 hunks)components/core/tools/docker-images/clp-core-ubuntu-noble/build.sh(1 hunks)components/core/tools/docker-images/clp-env-base-ubuntu-noble/Dockerfile(1 hunks)components/core/tools/docker-images/clp-env-base-ubuntu-noble/build.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-noble/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-noble/install-packages-from-source.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-noble/install-prebuilt-packages.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/clp-core-build.yaml (1)
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.500Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
🪛 Checkov (3.2.334)
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
[LOW] 1-29: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-29: Ensure that a user for the container has been created
(CKV_DOCKER_3)
components/core/tools/docker-images/clp-env-base-ubuntu-noble/Dockerfile
[LOW] 1-16: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-16: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
[warning] 5-5: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[info] 5-5: Avoid additional packages by specifying --no-install-recommends
(DL3015)
🪛 actionlint (1.7.7)
.github/workflows/clp-core-build.yaml
168-168: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
168-168: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
309-309: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
309-309: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
422-422: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
422-422: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
551-551: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
551-551: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
🔇 Additional comments (6)
components/core/tools/scripts/lib_install/ubuntu-noble/install-packages-from-source.sh (1)
4-8: Great addition of strict error handling flags
Includingset -e,set -u, andset -o pipefailensures the script aborts on failures and undefined variables, giving robust pipeline error detection..github/workflows/clp-core-build.yaml (5)
49-49: Include Ubuntu Noble in the filter outputs
Addingubuntu_noble_image_changedkeeps the new variant on par withubuntu_jammyandcentos_stream_9. Ensure downstream jobs correctly reference this output.
157-182: Consistent deps-image job for Ubuntu Noble
Thisubuntu-noble-deps-imagejob aligns with theubuntu-jammyand CentOS Stream 9 variants, using the same actions and context. Pattern looks good.🧰 Tools
🪛 actionlint (1.7.7)
168-168: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
168-168: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
283-343: Add binaries build for Ubuntu Noble
Theubuntu-noble-binariesjob correctly duplicates the matrix strategy and artifact handling of existing variants, ensuring consistent dynamic/static builds and uploads.🧰 Tools
🪛 actionlint (1.7.7)
309-309: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
309-309: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
407-469: Publish binaries Docker image for Ubuntu Noble
This job mirrors theubuntu-jammy-binaries-imagesetup, including metadata tagging and conditional publishing. It’s consistent and clear.🧰 Tools
🪛 actionlint (1.7.7)
422-422: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
422-422: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
533-594: Enable lint checks on Ubuntu Noble
Theubuntu-noble-lintjob properly reuses cache steps and run-on-image logic fromubuntu-jammy-lint. All lint tasks are now covered.🧰 Tools
🪛 actionlint (1.7.7)
551-551: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
551-551: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-core-ubuntu-noble/Dockerfile
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-env-base-ubuntu-noble/build.sh
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-env-base-ubuntu-noble/build.sh
Outdated
Show resolved
Hide resolved
components/core/tools/docker-images/clp-core-ubuntu-noble/build.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/ubuntu-noble/install-packages-from-source.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/ubuntu-noble/install-packages-from-source.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/ubuntu-noble/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/ubuntu-noble/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/clp-core-build.yaml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/clp-core-build.yaml
[error] 376-376: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (2)
.github/workflows/clp-core-build.yaml (2)
181-187: Introduce dynamic parallelism for CentOS binaries build
Usinggetconf _NPROCESSORS_ONLNto setCLP_CORE_MAX_PARALLELISM_PER_BUILD_TASKis a solid improvement for performance scalability. This aligns with the PR goal of optimizing builds across environments.
225-231: Enable dynamic parallelism for Ubuntu Jammy binaries build
Mirroring the CentOS setup, injectingCLP_CORE_MAX_PARALLELISM_PER_BUILD_TASKhere ensures consistent performance tuning across images.
quinntaylormitchell
changed the title
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
2 participants
Description
An execution image for Ubuntu Noble is now built by modifying the preexisting execution image build workflow and adding the relevant directories/files in
tools/docker-imagesto support building the Docker image. In addition, changes mariadb python connection version to >=1.1.0, <1.2.0 so that mariadb is noble-compatible. Changes mention ofmariadb.connectionobject tomariadb.Connectionto comply with new standards in aforementioned versions.Checklist
breaking change.
Validation performed
All workflows passed. Built and ran package on Noble vm; to accomplish that, I changed and then reverted a line in
load_execution_container_name()inclp_config.py, as the execution image did not exist while testing. This PR creates the workflow that will publish theclp-execution-x86-ubuntu-noble:mainimage.Summary by CodeRabbit