-
Notifications
You must be signed in to change notification settings - Fork 91
feat(docker): Add container image containing the CLP package, for Docker Compose integration (resolves #1164). #1166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
8ea4529
4d402c1
adef439
9aa74ff
542da81
ce43d17
2c2ee50
75f88d9
414e1fb
0e9138a
912b678
dcc8a70
46ffa55
9db7ca9
d609987
1b3959d
1f13954
aceb39f
c49d8b4
099afc0
73ca1a0
a4317ea
806f2f8
1887626
20dcfb1
ddab8d1
92861b9
b39644e
c17a782
8ff64f2
1dfe706
47d0517
628ddae
23f1ef2
63a7ec3
61da837
227d6dd
e98bb8e
e6f1af5
860e5ef
4180739
6cfabd7
f1c566f
09539ae
b49daba
be2560e
44a2dc5
2ebbfe0
1254827
62cf06a
2f29228
c413a13
bb36035
ea4ce51
3c81014
e5a66cf
ff21c8e
cbea39b
a857701
7dfbcc5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,8 +1,14 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: "clp-execution-image-build" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Builds a container image that contains the dependencies necessary | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| to run the CLP package." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: "clp-image-build" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Builds a CLP container image." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
junhaoliao marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| inputs: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| image_type: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Type of image to build" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| required: true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: "choice" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| options: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - "execution" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - "package" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
kirkrodrigues marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| image_registry: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default: "ghcr.io" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description: "Container image registry" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -50,15 +56,29 @@ runs: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: "docker/metadata-action@902fa8ec7d6ecbf8d84d538b9b233a880e428804" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| images: "${{inputs.image_registry}}/${{steps.sanitization.outputs.REPOSITORY}}\ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /clp-execution-x86-${{inputs.platform_id}}-${{inputs.platform_version_id}}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /clp-${{inputs.image_type}}-x86-${{inputs.platform_id}}-${{inputs.platform_version_id}}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hard-coded The - /clp-${{inputs.image_type}}-x86-${{inputs.platform_id}}-${{inputs.platform_version_id}}"
+ /clp-${{inputs.image_type}}-${{inputs.platform_id}}-${{inputs.platform_version_id}}"(Alternatively inject 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: "Determine Dockerfile Path" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: "dockerfile" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| shell: "bash" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| base_path="./tools/docker-images" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| platform="${{inputs.platform_id}}-${{inputs.platform_version_id}}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if [[ "${{inputs.image_type}}" == "execution" ]]; then | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| path="$base_path/clp-execution-base-$platform/Dockerfile" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about dockerfile_path to avoid confusion with "$PATH"? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| path="$base_path/clp-package-$platform/Dockerfile" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "DOCKERFILE=$path" >> "$GITHUB_OUTPUT" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Harden path resolution: quote expansions, fail fast, validate Unquoted variables and missing existence checks can lead to brittle failures. Also, Apply: - - name: "Determine Dockerfile Path"
+ - name: "Determine Dockerfile Path"
id: "dockerfile"
shell: "bash"
run: |
- base_path="./tools/docker-images"
- platform="${{inputs.platform_id}}-${{inputs.platform_version_id}}"
-
- if [[ "${{inputs.image_type}}" == "execution" ]]; then
- path="$base_path/clp-execution-base-$platform/Dockerfile"
- else
- path="$base_path/clp-package-$platform/Dockerfile"
- fi
-
- echo "DOCKERFILE=$path" >> "$GITHUB_OUTPUT"
+ set -euo pipefail
+ base_path="./tools/docker-images"
+ platform="${{ inputs.platform_id }}-${{ inputs.platform_version_id }}"
+
+ case "${{ inputs.image_type }}" in
+ execution)
+ dockerfile_path="${base_path}/clp-execution-base-${platform}/Dockerfile"
+ ;;
+ package)
+ dockerfile_path="${base_path}/clp-package-${platform}/Dockerfile"
+ ;;
+ *)
+ echo "Unsupported image_type: '${{ inputs.image_type }}' (expected: execution|package)" >&2
+ exit 1
+ ;;
+ esac
+
+ if [[ ! -f "${dockerfile_path}" ]]; then
+ echo "Dockerfile not found at: ${dockerfile_path}" >&2
+ exit 1
+ fi
+
+ printf 'DOCKERFILE=%s\n' "${dockerfile_path}" >> "$GITHUB_OUTPUT"This addresses quoting, validation, and verifiable failure modes. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: "Build and Push" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if: "github.event_name != 'pull_request' && github.ref == 'refs/heads/main'" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: "docker/build-push-action@471d1dc4e07e5cdedd4c2171150001c434f0b7a4" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| context: "./" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| file: "./tools/docker-images/\ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp-execution-base-${{inputs.platform_id}}-${{inputs.platform_version_id}}/Dockerfile" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| file: "${{steps.dockerfile.outputs.DOCKERFILE}}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| push: true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tags: "${{steps.meta.outputs.tags}}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| labels: "${{steps.meta.outputs.labels}}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,103 @@ | ||||||||||||||||||||||||||
| name: "clp-package-image-build" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||
| push: | ||||||||||||||||||||||||||
| workflow_dispatch: | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Trigger mismatch: workflow doesn’t run on pull_request but contains PR-specific logic The use_published_image condition branches on github.event_name == 'pull_request', but the workflow isn’t triggered by pull_request. If you intend PR builds (without pushing), add a pull_request trigger, ideally with the same path filtering strategy you use elsewhere. Proposed minimal addition: on:
push:
+ pull_request:
workflow_dispatch:If PR builds are intentionally disabled for cost/safety, consider removing the pull_request branch in the predicate to avoid confusion. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||
| DEPS_IMAGE_NAME_PREFIX: "clp-core-dependencies-x86-" | ||||||||||||||||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Similar to the comment at https://github.com/y-scope/clp/pull/1166/files#r2256441706) I see how it can be awkward to build the package with a "core" dependencies container, but i haven't seen any quicker way to build the package. if there's no significant benefit in differentiating between "core" and "package" dependencies, shall we just unify them (simply rename the existing build env containers).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think we can rename the existing containers and make them sufficient for building the entire CLP package (since the requirements for building the package are not a lot more than what's required for building core). That said, let's defer this to another PR to avoid delaying this one.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai create an issue to track the effort of creating a universal build env image
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@junhaoliao I've created a GitHub issue to track the effort of creating a universal build environment image. The issue captures the context from your discussion with kirkrodrigues about unifying the "core" and "package" dependencies containers, includes references to this PR and the specific comment, and outlines the proposed solution with clear acceptance criteria. 🏁 Script executed: Length of output: 1491 @junhaoliao Perfect! I've successfully created issue #1266 to track the effort of creating a universal build environment image. The issue is now available at #1266 and has been assigned to you as requested. The issue captures all the relevant context from your discussion with kirkrodrigues about unifying the "core" and "package" dependencies containers, includes proper references to this PR and the specific comment thread, and provides clear acceptance criteria for completing the work. 🧠 Learnings used |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| jobs: | ||||||||||||||||||||||||||
| filter-relevant-changes: | ||||||||||||||||||||||||||
| name: "filter-relevant-changes" | ||||||||||||||||||||||||||
| runs-on: "ubuntu-24.04" | ||||||||||||||||||||||||||
| outputs: | ||||||||||||||||||||||||||
| ubuntu_jammy_image_changed: "${{steps.filter.outputs.ubuntu_jammy_image}}" | ||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||
| - uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| submodules: "recursive" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| - name: "Work around actions/runner-images/issues/6775" | ||||||||||||||||||||||||||
| run: "chown $(id -u):$(id -g) -R ." | ||||||||||||||||||||||||||
| shell: "bash" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Quote The workaround lines can trigger word-splitting warnings; quoting fixes it. - run: "chown $(id -u):$(id -g) -R ."
+ run: "chown $(id -u):$(id -g) -R ."(Only quotes needed around the whole expression if re-templated; keep consistent across all three occurrences.)
🧰 Tools🪛 actionlint (1.7.7)22-22: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting (shellcheck) 22-22: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting (shellcheck) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| - name: "Filter relevant changes" | ||||||||||||||||||||||||||
| uses: "dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36" | ||||||||||||||||||||||||||
| id: "filter" | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| # Consider changes between the current commit and `main` | ||||||||||||||||||||||||||
| # NOTE: If a pull request changes one of the images, then we need to (1) build the image | ||||||||||||||||||||||||||
| # (based on commits in the PR) and then (2) build CLP using the changed image. If a pull | ||||||||||||||||||||||||||
| # request doesn't change an image, then we don't need to rebuild the image; instead we can | ||||||||||||||||||||||||||
| # use the published image which is based on `main`. So when determining what files have | ||||||||||||||||||||||||||
| # changed, we need to consider the delta between the current commit and `main` (rather | ||||||||||||||||||||||||||
| # than the current and previous commits) in order to detect if we need to rebuild the | ||||||||||||||||||||||||||
| # image (since it would be different from the published image). | ||||||||||||||||||||||||||
| base: "main" | ||||||||||||||||||||||||||
| filters: | | ||||||||||||||||||||||||||
| ubuntu_jammy_image: | ||||||||||||||||||||||||||
| - ".github/actions/**" | ||||||||||||||||||||||||||
| - ".github/workflows/clp-core-build.yaml" | ||||||||||||||||||||||||||
| - "components/core/tools/scripts/lib_install/*.sh" | ||||||||||||||||||||||||||
| - "components/core/tools/docker-images/clp-env-base-ubuntu-jammy/**" | ||||||||||||||||||||||||||
| - "components/core/tools/scripts/lib_install/ubuntu-jammy/**" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||
| ubuntu-jammy-deps-image: | ||||||||||||||||||||||||||
| name: "ubuntu-jammy-deps-image" | ||||||||||||||||||||||||||
| needs: "filter-relevant-changes" | ||||||||||||||||||||||||||
| runs-on: "ubuntu-24.04" | ||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||
| - uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| submodules: "recursive" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| - name: "Work around actions/runner-images/issues/6775" | ||||||||||||||||||||||||||
| run: "chown $(id -u):$(id -g) -R ." | ||||||||||||||||||||||||||
| shell: "bash" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Quote command substitutions here as well Same rationale as above. - - name: "Work around actions/runner-images/issues/6775"
- run: "chown $(id -u):$(id -g) -R ."
+ - name: "Work around actions/runner-images/issues/6775"
+ run: "chown \"$(id -u)\":\"$(id -g)\" -R ."
shell: "bash"📝 Committable suggestion
Suggested change
🧰 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) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| - uses: "./.github/actions/clp-core-build-containers" | ||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||
| OS_NAME: "ubuntu-jammy" | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" | ||||||||||||||||||||||||||
| docker_context: "components/core" | ||||||||||||||||||||||||||
| docker_file: "components/core/tools/docker-images/clp-env-base-${{env.OS_NAME}}\ | ||||||||||||||||||||||||||
| /Dockerfile" | ||||||||||||||||||||||||||
| push_deps_image: >- | ||||||||||||||||||||||||||
| ${{github.event_name != 'pull_request' && github.ref == 'refs/heads/main'}} | ||||||||||||||||||||||||||
| token: "${{secrets.GITHUB_TOKEN}}" | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is a good idea since it might cause some issues with two different workflows trying to publish the same container, not to mention the resources it'll use. I think a quick solution might be to move this package image build into clp-core-build workflow.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
if we move it into clp-core-build, i believe we need to update the workflows trigger conditions. instead of an allow list, we might need to write it as then the other jobs in the workflow will rely on is my understanding correct? are there any other paths / patterns that need to be added to the deny list (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. That pattern should be fine. |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ubuntu-jammy-package-image: | ||||||||||||||||||||||||||
| name: "ubuntu-jammy-package-image" | ||||||||||||||||||||||||||
| needs: "ubuntu-jammy-deps-image" | ||||||||||||||||||||||||||
| runs-on: "ubuntu-24.04" | ||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Job cannot reference
-needs: "ubuntu-jammy-deps-image"
+needs:
+ - "ubuntu-jammy-deps-image"
+ - "filter-relevant-changes"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| OS_NAME: "ubuntu-jammy" | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this into the scope of the step that requires it? |
||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||
| - uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| submodules: "recursive" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| - name: "Workaround actions/runner-images/issues/6775" | ||||||||||||||||||||||||||
| shell: "bash" | ||||||||||||||||||||||||||
| run: "chown $(id -u):$(id -g) -R ." | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| - name: "Build the package" | ||||||||||||||||||||||||||
| uses: "./.github/actions/run-on-image" | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" | ||||||||||||||||||||||||||
| use_published_image: >- | ||||||||||||||||||||||||||
| ${{needs.filter-relevant-changes.outputs.ubuntu_jammy_image_changed == 'false' | ||||||||||||||||||||||||||
| || (github.event_name != 'pull_request' && github.ref == 'refs/heads/main')}} | ||||||||||||||||||||||||||
| run_command: >- | ||||||||||||||||||||||||||
| CLP_CORE_MAX_PARALLELISM_PER_BUILD_TASK=$(getconf _NPROCESSORS_ONLN) task package | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Guardrails look good; minor readability nit The 🧰 Tools🪛 actionlint (1.7.7)91-91: property "filter-relevant-changes" is not defined in object type {ubuntu-jammy-deps-image: {outputs: {}; result: string}} (expression) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| - uses: "./.github/actions/clp-image-build" | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| image_type: "package" | ||||||||||||||||||||||||||
| image_registry: "ghcr.io" | ||||||||||||||||||||||||||
| image_registry_username: "${{github.actor}}" | ||||||||||||||||||||||||||
| image_registry_password: "${{secrets.GITHUB_TOKEN}}" | ||||||||||||||||||||||||||
| platform_id: "ubuntu" | ||||||||||||||||||||||||||
| platform_version_id: "jammy" | ||||||||||||||||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i understand the added dependencies are not strictly "core" related but i believe some others here (e.g. unzip) are in a similar situation. we might want to split the package specific dependencies in anther refactoring PR, though i personally don't see too much benefit yet. any concerns / other ideas? |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -162,6 +162,17 @@ tasks: | |||||||||||||||||||||||||||||||||||||||||||||||||
| CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| INCLUDE_PATTERNS: ["{{.OUTPUT_DIR}}"] | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| docker-image-package: | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
kirkrodrigues marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| vars: | ||||||||||||||||||||||||||||||||||||||||||||||||||
| SRC_DIR: "{{.ROOT_DIR}}/tools/docker-images/clp-package-ubuntu-jammy" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| dir: "{{.SRC_DIR}}" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| deps: | ||||||||||||||||||||||||||||||||||||||||||||||||||
| - "package" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| sources: | ||||||||||||||||||||||||||||||||||||||||||||||||||
| - "{{.SRC_DIR}}/**/*" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| cmds: | ||||||||||||||||||||||||||||||||||||||||||||||||||
| - "./build.sh" | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainMake docker-image-package sensitive to package output changes and check Docker availability Current sources only watch the Docker image directory. Since the build script uses the repo root as the Docker build context and the image embeds the freshly built package (deps: package), the task can be incorrectly considered up-to-date when only package outputs change. Also, adding a precondition to ensure Docker is installed will fail fast with a clear message. Proposed fixes:
Apply this diff: docker-image-package:
vars:
SRC_DIR: "{{.ROOT_DIR}}/tools/docker-images/clp-package-ubuntu-jammy"
dir: "{{.SRC_DIR}}"
deps:
- "package"
sources:
- "{{.SRC_DIR}}/**/*"
+ - "{{.G_PACKAGE_BUILD_DIR}}/**/*"
+ preconditions:
+ - sh: "command -v docker"
+ msg: "Docker is required to build the package image. Please install Docker and try again."
cmds:
- "./build.sh"Follow-up:
🏁 Script executed: #!/bin/bash
set -e
echo "1. Searching for G_PACKAGE_BUILD_DIR in YAML files..."
rg -n 'G_PACKAGE_BUILD_DIR' --glob '*.yaml' --glob '*.yml' || true
echo "2. Extracting 'package' task from Taskfile.yaml..."
rg -n '^[ \\t]*package:' -n -C10 taskfile.yaml || true
echo "3. Checking for existing 'preconditions' usage..."
rg -n 'preconditions:' -n -C3 taskfile.yaml || true
echo "4. Showing COPY instructions in Dockerfile..."
grep -n -C2 'COPY' tools/docker-images/clp-package-ubuntu-jammy/Dockerfile || trueLength of output: 2018 Ensure docker-image-package rebuilds on package changes and verifies Docker availability To prevent stale images when only the package output changes and to fail fast if Docker isn’t installed, update the • Add the built package directory to Apply this diff: docker-image-package:
vars:
SRC_DIR: "{{.ROOT_DIR}}/tools/docker-images/clp-package-ubuntu-jammy"
dir: "{{.SRC_DIR}}"
deps:
- "package"
sources:
- "{{.SRC_DIR}}/**/*"
+ - "{{.G_PACKAGE_BUILD_DIR}}/**/*"
+ preconditions:
+ - sh: "command -v docker"
+ msg: "Docker is required to build the package image. Please install Docker and try again."
cmds:
- "./build.sh"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
| clp-s-generate-parsers: | ||||||||||||||||||||||||||||||||||||||||||||||||||
| vars: | ||||||||||||||||||||||||||||||||||||||||||||||||||
| CHECKSUM_FILE: "{{.G_BUILD_DIR}}/{{.TASK}}.md5" | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,22 @@ | ||||
| FROM ubuntu:jammy AS base | ||||
|
|
||||
| COPY ./build/clp-package /opt/clp | ||||
|
|
||||
| WORKDIR /root | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) WORKDIR in base doesn’t carry into the final stage Since you re-stage with -WORKDIR /root📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||
|
|
||||
| COPY ./tools/docker-images/clp-package-ubuntu-jammy/setup-scripts ./setup-scripts | ||||
| RUN ./setup-scripts/install-prebuilt-packages.sh | ||||
| RUN rm -rf ./setup-scripts/ | ||||
|
|
||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||
| # Remove cached files | ||||
| RUN apt-get clean \ | ||||
| && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* | ||||
|
|
||||
| # Flatten the image | ||||
| FROM scratch | ||||
| COPY --from=base / / | ||||
|
|
||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||
| ENV PATH="/opt/clp/sbin:${PATH}" | ||||
| ENV PATH="/opt/clp/bin:${PATH}" | ||||
| ENV PYTHONPATH="/opt/clp/lib/python3/site-packages" | ||||
| ENV CLP_HOME="/opt/clp" | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Combine PATH modifications into a single ENV and preserve original order Multiple -ENV PATH="/opt/clp/sbin:${PATH}"
-ENV PATH="/opt/clp/bin:${PATH}"
+ENV PATH="/opt/clp/sbin:/opt/clp/bin:${PATH}"🧰 Tools🪛 Checkov (3.2.334)[LOW] 1-22: Ensure that HEALTHCHECK instructions have been added to container images (CKV_DOCKER_2) [LOW] 1-22: Ensure that a user for the container has been created (CKV_DOCKER_3) 🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't splitting into multiple lines be more VCS friendly?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @junhaoliao You raise an excellent point about VCS friendliness! You're absolutely right that separate ENV lines make it much easier to track changes in version control - when someone adds or removes a path component, it shows up as a clean line addition/removal rather than a modification to an entire concatenated string. The trade-off is:
Given that this is a new Dockerfile and you're prioritizing maintainability and clear change tracking, keeping the separate ENV lines is a perfectly reasonable choice. The Docker layer efficiency gain is minimal compared to the VCS clarity benefit you mentioned. I'll defer to your preference on this one! 🐰 🧠 Learnings used
junhaoliao marked this conversation as resolved.
Outdated
|
||||
|
kirkrodrigues marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||||||||||||||||||||||||||||||||||
| #!/usr/bin/env bash | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # Exit on any error | ||||||||||||||||||||||||||||||||||||||
| set -e | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # Error on undefined variable | ||||||||||||||||||||||||||||||||||||||
| set -u | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||||||||||||||||||||||||||||||||||||||
| repo_root=${script_dir}/../../../ | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| arch=$(uname -m) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if [ "$arch" = "x86_64" ]; then | ||||||||||||||||||||||||||||||||||||||
| arch_name="x86" | ||||||||||||||||||||||||||||||||||||||
| elif [ "$arch" = "aarch64" ]; then | ||||||||||||||||||||||||||||||||||||||
| arch_name="arm64" | ||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||
| echo "Error: Unsupported architecture - $arch" | ||||||||||||||||||||||||||||||||||||||
| exit 1 | ||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| build_cmd=( | ||||||||||||||||||||||||||||||||||||||
| docker build | ||||||||||||||||||||||||||||||||||||||
| --tag clp-package-${arch_name}-ubuntu-jammy:dev | ||||||||||||||||||||||||||||||||||||||
| "$repo_root" | ||||||||||||||||||||||||||||||||||||||
| --file "${script_dir}/Dockerfile" | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Comment on lines
+20
to
+
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Preflight: fail fast if the package payload is missing Avoids a confusing Docker build error when build_cmd=(
docker build
--tag "clp-package-${arch_name}-ubuntu-jammy:dev"
"$repo_root"
--file "${script_dir}/Dockerfile"
)
+
+# Ensure build payload exists
+if [ ! -d "${repo_root%/}/build/clp-package" ]; then
+ echo "Error: Missing build/clp-package. Run 'task package' first." >&2
+ exit 1
+fi📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if command -v git >/dev/null && git -C "$script_dir" 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)" | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Use repository root for Git labels to stay consistent with other image builders Current call runs - --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)"🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| "${build_cmd[@]}" | ||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| # Exit on any error | ||
| set -e | ||
|
|
||
| # Error on undefined variable | ||
| set -u | ||
|
|
||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| apt-get update | ||
| DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ | ||
| ca-certificates \ | ||
| checkinstall \ | ||
| curl \ | ||
| libcurl4 \ | ||
| libmariadb-dev \ | ||
| libssl-dev \ | ||
| python3 \ | ||
| rsync \ | ||
| zstd | ||
Uh oh!
There was an error while loading. Please reload this page.