-
Notifications
You must be signed in to change notification settings - Fork 111
bootstrap: Small fixes around repository #3538
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
bootstrap: Small fixes around repository #3538
Conversation
Reviewer's GuideThis PR adds validation of Git-describe tags during the Docker build and updates bootstrap usage examples to correct image names and volume mount paths. Class diagram for Dockerfile build argument validationclassDiagram
class Dockerfile {
+ARG GIT_DESCRIBE_TAGS
+ENV GIT_DESCRIBE_TAGS
+RUN: Validate GIT_DESCRIBE_TAGS format
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the Dockerfile's version-validation RUN block into a separate script or file to improve readability and maintainability.
- The multi-line string concatenation in main.py for the docker run example is hard to read; using a single triple-quoted string or textwrap.dedent can simplify it.
- You may want to centralize the image name and default config paths as constants to avoid duplication between README, Dockerfile, and Python code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the Dockerfile's version-validation RUN block into a separate script or file to improve readability and maintainability.
- The multi-line string concatenation in main.py for the docker run example is hard to read; using a single triple-quoted string or textwrap.dedent can simplify it.
- You may want to centralize the image name and default config paths as constants to avoid duplication between README, Dockerfile, and Python code.
## Individual Comments
### Comment 1
<location> `bootstrap/Dockerfile:7` </location>
<code_context>
+# The format can be invalid if someone is build BlueOS without a tag as reference
+ARG GIT_DESCRIBE_TAGS
+ENV GIT_DESCRIBE_TAGS=${GIT_DESCRIBE_TAGS:-0.0.0-0-g00000000}
+RUN <<-EOF
+set -e
+
+ if [ -z "$GIT_DESCRIBE_TAGS" ]; then
+ echo "GIT_DESCRIBE_TAGS argument not provided."
+ echo "Use: --build-arg GIT_DESCRIBE_TAGS=\$(git describe --tags --long --always)"
+ exit 1
+ fi
+
+ if [[ ! $GIT_DESCRIBE_TAGS =~ -[0-9]+-g[a-f0-9]{8}$ ]]; then
+ echo "Invalid format: $GIT_DESCRIBE_TAGS (E.g: <TAG>-<COMMIT_NUMBER>-g<SHORT_HASH>)"
+ exit 1
+ fi
+
+EOF
+
COPY startup.json.default bootstrap/ /bootstrap/
</code_context>
<issue_to_address>
The Dockerfile uses bash-specific syntax (e.g., [[ ... ]]) which may not be supported by /bin/sh.
To ensure compatibility, either specify bash explicitly in the RUN command or replace [[ ... ]] with POSIX-compliant [ ... ] syntax.
</issue_to_address>
### Comment 2
<location> `bootstrap/main.py:20` </location>
<code_context>
logger.info("Example docker command line:")
logger.info(
"docker run -it -v /var/run/docker.sock:/var/run/docker.sock"
- " -v $HOME/.config/blueos:"
+ " -v $HOME/.config/blueos:/root/.config/blueos"
" -v /var/logs/blueos:/var/logs/blueos"
- "/root/.config/blueos -e BLUEOS_CONFIG_PATH=$HOME/.config/blueos"
</code_context>
<issue_to_address>
The volume mapping now explicitly maps $HOME/.config/blueos to /root/.config/blueos.
This mapping only works if the container runs as root. Please document or address cases where a different user is used.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
RUN <<-EOF | ||
set -e | ||
|
||
if [ -z "$GIT_DESCRIBE_TAGS" ]; then | ||
echo "GIT_DESCRIBE_TAGS argument not provided." | ||
echo "Use: --build-arg GIT_DESCRIBE_TAGS=\$(git describe --tags --long --always)" | ||
exit 1 | ||
fi | ||
|
||
if [[ ! $GIT_DESCRIBE_TAGS =~ -[0-9]+-g[a-f0-9]{8}$ ]]; then |
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.
issue (bug_risk): The Dockerfile uses bash-specific syntax (e.g., [[ ... ]]) which may not be supported by /bin/sh.
To ensure compatibility, either specify bash explicitly in the RUN command or replace [[ ... ]] with POSIX-compliant [ ... ] syntax.
logger.info("Example docker command line:") | ||
logger.info( | ||
"docker run -it -v /var/run/docker.sock:/var/run/docker.sock" | ||
" -v $HOME/.config/blueos:" |
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.
suggestion: The volume mapping now explicitly maps $HOME/.config/blueos to /root/.config/blueos.
This mapping only works if the container runs as root. Please document or address cases where a different user is used.
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
It's set by default Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
2ab6297
to
6325ac0
Compare
Summary by Sourcery
Add validation for GIT_DESCRIBE_TAGS in the bootstrap Dockerfile and correct the docker run examples in code and documentation to use the proper mount path and image name
Enhancements: