feature: docker build and release workflow#573
feature: docker build and release workflow#573dougyster wants to merge 5 commits intoradixark:mainfrom
Conversation
Summary of ChangesHello @dougyster, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Docker image building and pushing process for the 'Miles' project. By introducing a new Python script, Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the justfile with a new Python script, docker/build.py, for building and pushing Docker images, improving structure and maintainability. However, it introduces an insecure temporary directory for the build cache and logs sensitive proxy credentials. Recommendations include moving the cache directory to a user-specific location, masking sensitive build arguments in logs, and improving command-line argument display, proxy environment variable handling, and path management for overall robustness and code quality.
|
|
||
|
|
||
| def run(cmd: list[str], dry_run: bool) -> None: | ||
| print(f"+ {' '.join(cmd)}", flush=True) |
There was a problem hiding this comment.
The script prints the full Docker command, including sensitive proxy credentials (HTTP_PROXY, HTTPS_PROXY), which are exposed in logs. This is a security vulnerability. Additionally, using ' '.join(cmd) can misrepresent the command for logging if arguments contain spaces. It's more robust to use shlex.join() for correct quoting and improved debuggability. Consider masking sensitive values before printing and ensure shlex is imported.
| print(f"+ {' '.join(cmd)}", flush=True) | |
| print(f"+ {shlex.join(cmd)}", flush=True) |
| import subprocess | ||
| from pathlib import Path | ||
|
|
||
| CACHE_DIR = "/tmp/miles-docker-cache" |
There was a problem hiding this comment.
The use of a hardcoded, predictable directory in /tmp for the Docker build cache is insecure. Since /tmp is world-writable, a local attacker could pre-create this directory or use symlinks to trick the script (especially if run with elevated privileges, which is common for Docker builds) into writing data to arbitrary locations on the filesystem. Consider using a user-specific cache directory.
| CACHE_DIR = "/tmp/miles-docker-cache" | |
| CACHE_DIR = os.path.expanduser("~/.cache/miles-docker-cache") |
|
|
||
| def build_and_push(variant: str, dry_run: bool) -> None: | ||
| config = VARIANTS[variant] | ||
| repo_root = Path(__file__).resolve().parent.parent |
There was a problem hiding this comment.
For better code structure and to make functions more self-contained, consider defining repo_root as a module-level constant.
- Add this after your imports:
REPO_ROOT = Path(__file__).resolve().parent.parent
- Update
get_versionto use this constant and remove itsrepo_rootparameter:def get_version() -> str: version_file = REPO_ROOT / "docker" / "version.txt" return version_file.read_text().strip()
- Finally, you can remove this line (
repo_root = ...) and callget_version()without arguments on line 72.
| for env_var, arg_name in [ | ||
| ("http_proxy", "HTTP_PROXY"), | ||
| ("https_proxy", "HTTPS_PROXY"), | ||
| ]: | ||
| value = os.environ.get(env_var, "") | ||
| if value: | ||
| cmd += ["--build-arg", f"{arg_name}={value}"] |
There was a problem hiding this comment.
The current proxy handling only checks for lowercase environment variables (http_proxy, https_proxy). It is a common convention to also support uppercase versions (HTTP_PROXY, HTTPS_PROXY). To make the script more robust, you should check for both.
| for env_var, arg_name in [ | |
| ("http_proxy", "HTTP_PROXY"), | |
| ("https_proxy", "HTTPS_PROXY"), | |
| ]: | |
| value = os.environ.get(env_var, "") | |
| if value: | |
| cmd += ["--build-arg", f"{arg_name}={value}"] | |
| for arg_name in ["HTTP_PROXY", "HTTPS_PROXY"]: | |
| value = os.environ.get(arg_name.lower()) or os.environ.get(arg_name) | |
| if value: | |
| cmd += ["--build-arg", f"{arg_name}={value}"] |
dougyster
requested review from
fzyzcjy,
guapisolo,
maocheng23 and
yueming-yuan
as code owners
dougyster
force-pushed
the
justfile-gh-workflow
branch
from
04680d2 to
40b81fa
Compare
1 participant
Created build.py based on justfile to get the correct tags and build args for each build type and added a dry-run flag to preview args before building.
Added dockerfile path into the workflow to specify which dockerfile to build.