Conversation
Contributor
Reviewer's GuideAdds a new GitHub Actions workflow that builds a Docker image using docker compose, slims it with SlimToolkit, and pushes tagged images to Docker Hub on every push. Flow diagram for build-and-push Docker image job in GitHub Actionsgraph TD
A[Push event to repository] --> B[Trigger workflow Build and Push Docker Images]
B --> C[Start job build-eagle-slim on ubuntu-latest]
C --> D[Checkout repository using actions/checkout@v4]
D --> E[Extract Git tag from GITHUB_REF_NAME into VCS_TAG]
E --> F[Log in to Docker Hub using secrets DOCKERHUB_USERNAME and DOCKERHUB_TOKEN]
F --> G[Run docker compose -f docker/docker-compose.dep.yml build]
G --> H[Install SlimToolkit via install-slim.sh]
H --> I[Run slim build to create icrar/eagle.slim:VCS_TAG from icrar/eagle:VCS_TAG]
I --> J[docker push --all-tags icrar/eagle.slim to Docker Hub]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Collaborator
|
Hi @myxie, we were having test failure issues across all our branches. Once I fixed that in master, I merged the changes into this branch. Sure enough, it fixed the tests. Was the failing tests the reason this PR is a draft, or is there something else still in-progress? |
Collaborator
Author
|
@james-strauss-uwa I was hoping to get some feedback from the team and @awicenec on how you wanted this to work moving forward, but Andreas was away and then I have been focused on AusSRC. If you're happy with this as a preliminary implementation, I can update the scripts so it only generates on release and then open this for review, probably from @awicenec given they're the most familiar with the existing containerisation? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The workflow currently runs on every push; consider scoping the trigger to tag pushes or specific branches so Docker Hub uploads only happen for intended releases.
- The
slim buildcommand assumes anicrar/eagle:${VCS_TAG}image already exists, but the workflow only runsdocker compose ... buildwithout an explicit tag or push; confirm the compose config tags that image as expected or add an explicitdocker build/docker tagstep. - Instead of installing Slim on every run via
curl | bash, consider pinning to a specific version or using a maintained GitHub Action/container image to improve reproducibility and reduce supply-chain risk.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The workflow currently runs on every push; consider scoping the trigger to tag pushes or specific branches so Docker Hub uploads only happen for intended releases.
- The `slim build` command assumes an `icrar/eagle:${VCS_TAG}` image already exists, but the workflow only runs `docker compose ... build` without an explicit tag or push; confirm the compose config tags that image as expected or add an explicit `docker build`/`docker tag` step.
- Instead of installing Slim on every run via `curl | bash`, consider pinning to a specific version or using a maintained GitHub Action/container image to improve reproducibility and reduce supply-chain risk.
## Individual Comments
### Comment 1
<location path=".github/workflows/docker-upload.yml" line_range="14-15" />
<code_context>
+ - name: Checkout Repository
+ uses: actions/checkout@v4
+
+ - name: Extract Git Tag
+ run: |
+ echo "VCS_TAG=${GITHUB_REF_NAME}" >> $GITHUB_ENV
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `GITHUB_REF_NAME` directly may produce unexpected tags for non-tag pushes.
On branch pushes, this will set `VCS_TAG` to the branch name (e.g., `main`), so images get tagged with branch names and `icrar/eagle:${VCS_TAG}` may not exist if base images are only built from tags.
If you only want to build/push on tag pushes, you could either restrict the workflow trigger to tag refs and keep `GITHUB_REF_NAME`, or compute `VCS_TAG` differently for branches (e.g., append the commit SHA) so base image/tag assumptions remain valid.
Suggested implementation:
```
on:
push:
tags:
- '*'
# release:
# types: [published]
```
1. Keep the existing `Extract Git Tag` step that sets `VCS_TAG=${GITHUB_REF_NAME}`; with this change the workflow will only run for tag pushes, so `GITHUB_REF_NAME` will always be a tag and the value of `VCS_TAG` will match your base image tags.
2. If you want to narrow which tags trigger the workflow (e.g., only semantic versions), replace `- '*'` with a more specific pattern such as `- 'v*'`.
</issue_to_address>
### Comment 2
<location path=".github/workflows/docker-upload.yml" line_range="27" />
<code_context>
+ - name: Build and Push eagle-slim
+ run: |
+ docker compose -f ./docker/docker-compose.dep.yml build
+ curl -sL https://raw.githubusercontent.com/slimtoolkit/slim/master/scripts/install-slim.sh | sudo -E bash -
+ slim build --include-shell --include-path /usr/local/lib --include-path /usr/local/bin --tag icrar/eagle.slim:${VCS_TAG} icrar/eagle:${VCS_TAG}
+ # docker push --all-tags icrar/eagle
</code_context>
<issue_to_address>
**🚨 issue (security):** Piping `curl` directly to `sudo bash` is a security risk and makes debugging harder.
Fetching and executing a remote script with `sudo` in a single step prevents inspection/validation and couples the build to whatever is at that URL at run time. Prefer downloading the script first, verifying it (e.g., checksum or pinned ref), then executing it, and pin slimtoolkit to a specific version/commit for reproducible builds.
</issue_to_address>
### Comment 3
<location path=".github/workflows/docker-upload.yml" line_range="26" />
<code_context>
+
+ - name: Build and Push eagle-slim
+ run: |
+ docker compose -f ./docker/docker-compose.dep.yml build
+ curl -sL https://raw.githubusercontent.com/slimtoolkit/slim/master/scripts/install-slim.sh | sudo -E bash -
+ slim build --include-shell --include-path /usr/local/lib --include-path /usr/local/bin --tag icrar/eagle.slim:${VCS_TAG} icrar/eagle:${VCS_TAG}
</code_context>
<issue_to_address>
**suggestion (performance):** Building all services via `docker compose ... build` may be more work than necessary for this job.
If only the slim-related images are needed, consider building just those services, e.g. `docker compose -f ./docker/docker-compose.dep.yml build <service-name>`. This can cut CI build time and resource usage and make the slim step’s dependencies explicit.
Suggested implementation:
```
- name: Build and Push eagle-slim
run: |
# Build only the service that produces the icrar/eagle:${VCS_TAG} image,
# to avoid building unrelated services and keep this job lean.
docker compose -f ./docker/docker-compose.dep.yml build eagle
curl -sL https://raw.githubusercontent.com/slimtoolkit/slim/master/scripts/install-slim.sh | sudo -E bash -
slim build --include-shell --include-path /usr/local/lib --include-path /usr/local/bin --tag icrar/eagle.slim:${VCS_TAG} icrar/eagle:${VCS_TAG}
# docker push --all-tags icrar/eagle
docker push --all-tags icrar/eagle.slim
```
1. Replace `eagle` in the `docker compose ... build eagle` command with the actual service name from `docker/docker-compose.dep.yml` that builds the `icrar/eagle:${VCS_TAG}` image (e.g. `eagle`, `eagle-app`, etc.).
2. If multiple services are required for the slim build (e.g. a base image and an app image), list them all: `docker compose -f ./docker/docker-compose.dep.yml build service1 service2`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+14
to
+15
| - name: Extract Git Tag | ||
| run: | |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Using GITHUB_REF_NAME directly may produce unexpected tags for non-tag pushes.
On branch pushes, this will set VCS_TAG to the branch name (e.g., main), so images get tagged with branch names and icrar/eagle:${VCS_TAG} may not exist if base images are only built from tags.
If you only want to build/push on tag pushes, you could either restrict the workflow trigger to tag refs and keep GITHUB_REF_NAME, or compute VCS_TAG differently for branches (e.g., append the commit SHA) so base image/tag assumptions remain valid.
Suggested implementation:
on:
push:
tags:
- '*'
# release:
# types: [published]
- Keep the existing
Extract Git Tagstep that setsVCS_TAG=${GITHUB_REF_NAME}; with this change the workflow will only run for tag pushes, soGITHUB_REF_NAMEwill always be a tag and the value ofVCS_TAGwill match your base image tags. - If you want to narrow which tags trigger the workflow (e.g., only semantic versions), replace
- '*'with a more specific pattern such as- 'v*'.
| - name: Build and Push eagle-slim | ||
| run: | | ||
| docker compose -f ./docker/docker-compose.dep.yml build | ||
| curl -sL https://raw.githubusercontent.com/slimtoolkit/slim/master/scripts/install-slim.sh | sudo -E bash - |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Piping curl directly to sudo bash is a security risk and makes debugging harder.
Fetching and executing a remote script with sudo in a single step prevents inspection/validation and couples the build to whatever is at that URL at run time. Prefer downloading the script first, verifying it (e.g., checksum or pinned ref), then executing it, and pin slimtoolkit to a specific version/commit for reproducible builds.
|
|
||
| - name: Build and Push eagle-slim | ||
| run: | | ||
| docker compose -f ./docker/docker-compose.dep.yml build |
Contributor
There was a problem hiding this comment.
suggestion (performance): Building all services via docker compose ... build may be more work than necessary for this job.
If only the slim-related images are needed, consider building just those services, e.g. docker compose -f ./docker/docker-compose.dep.yml build <service-name>. This can cut CI build time and resource usage and make the slim step’s dependencies explicit.
Suggested implementation:
- name: Build and Push eagle-slim
run: |
# Build only the service that produces the icrar/eagle:${VCS_TAG} image,
# to avoid building unrelated services and keep this job lean.
docker compose -f ./docker/docker-compose.dep.yml build eagle
curl -sL https://raw.githubusercontent.com/slimtoolkit/slim/master/scripts/install-slim.sh | sudo -E bash -
slim build --include-shell --include-path /usr/local/lib --include-path /usr/local/bin --tag icrar/eagle.slim:${VCS_TAG} icrar/eagle:${VCS_TAG}
# docker push --all-tags icrar/eagle
docker push --all-tags icrar/eagle.slim
- Replace
eaglein thedocker compose ... build eaglecommand with the actual service name fromdocker/docker-compose.dep.ymlthat builds theicrar/eagle:${VCS_TAG}image (e.g.eagle,eagle-app, etc.). - If multiple services are required for the slim build (e.g. a base image and an app image), list them all:
docker compose -f ./docker/docker-compose.dep.yml build service1 service2.
Collaborator
|
@myxie Sounds good |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://icrar.atlassian.net/browse/EAGLE-649
Summary by Sourcery
Build: