-
Notifications
You must be signed in to change notification settings - Fork 119
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
Provide dockerx setup for docker builds #1194
base: main
Are you sure you want to change the base?
Conversation
…tform docker builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 19d2d98 in 21 seconds
More details
- Looked at
34
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. ui/buildx_and_push.sh:7
- Draft comment:
Ensure thatjq
is installed before using it to parse JSON. You can add a check at the beginning of the script to verify its presence and provide a helpful error message if it's missing. Additionally, consider adding error handling for thecurl
command to ensure the script behaves correctly if the request to PyPI fails. For example, check if the response is empty or invalid and handle it appropriately. - Reason this comment was not posted:
Comment did not seem useful.
2. ui/buildx_and_push.sh:5
- Draft comment:
Consider adding this script to the Sphinx documentation underdocs/
to guide users on building and pushing multi-architecture Docker images. - Reason this comment was not posted:
Confidence changes required:50%
The script is straightforward and doesn't have repeated code, except for the docker buildx command which is slightly different for frontend and backend. The script is not overly complicated, so comments are not necessary beyond what is already provided. Variable names are descriptive. There are no secrets or credentials in the code. The script follows a single responsibility of building and pushing docker images. The function naming is consistent. Sensitive data is not logged. The script could be added to the documentation for users who want to build and push docker images.
Workflow ID: wflow_SV8PcasBZOtbB0DH
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
…rl response handling and docker buildx error handling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
what would a github action look like to get this to run once a new sf-hamilton-ui
version was published?
|
Sorry I'm not following. The multi-arch builds would just use your script here right? |
Yes. Just wanted to ask if you if the script is satisfying the requirements. |
Script looks great thanks. I haven't run it yet to verify though; so I assume it works right now. |
Hi @skrawcz . Ive created a new workflow, tested it on my fork, and it's building and pushing those images. So it's working fine. Let me know if I need to make any changes :) |
…t is located to prevent context related errors in workflow.
…with version tag from Dockerhub image.
…rsion from Docker Hub" step's shell script
Hi @skrawcz |
This address #1064.
A shell script was created to handle building and pushing multi-architecture images using docker buildx.
Changes
This PR introduces a shell script (
buildx_and_push.sh
) to automate the process of building and pushing Docker images for multiple architectures usingdocker buildx
. It ensures that the Docker images are always built with the latest version of thesf-hamilton-ui
library from PyPI.Key Changes:
Multi-architecture support:
linux/amd64
,linux/arm64
).Automated version management:
sf-hamilton-ui
version from PyPI.latest
tag are always in sync with the current release.Builder instance setup:
hamilton-builder
).Image tagging and pushing:
sf-hamilton-ui
version.latest
tag.How I Tested This
Environment:
Test Cases:
latest
tags were pushed to the repository.Validation:
dagworks/ui-backend:<fetched_version>
dagworks/ui-backend:latest
dagworks/ui-frontend:<fetched_version>
dagworks/ui-frontend:latest
Notes
Ensure Docker Buildx is installed before running the script. The setup logic in the script will handle creating the builder instance if it is not already configured.
Why this change is needed: This automation ensures consistency between the versioned tag and the
latest
tag. It also simplifies multi-architecture builds and makes them easier to manage across multiple platforms.PR has an informative and human-readable title (this will be pulled into the release notes)
Changes are limited to a single goal (no scope creep)
Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
Any change in functionality is tested
New functions are documented (with a description, list of inputs, and expected output)
Placeholder code is flagged / future TODOs are captured in comments
Project documentation has been updated if adding/changing functionality.