-
Notifications
You must be signed in to change notification settings - Fork 104
add branch and commit time info to image #737
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
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughExtends the CI/CD build process to capture additional Git metadata (commit timestamp and branch name) alongside the existing commit SHA, and integrates these new metadata fields into Docker image construction through corresponding Dockerfile modifications. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci/docker/Dockerfile (1)
98-98: LGTM! Metadata files expanded appropriately.The COPY command correctly includes the new
COMMIT_TIMEandBRANCHfiles alongside existing metadata. This maintains the single-layer approach for static files.Note: Manual builds of this Dockerfile will fail unless
COMMIT_TIMEandBRANCHfiles exist in the build context. Consider documenting this requirement or providing a script to generate these files for local development.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build_images.yamlci/docker/Dockerfile
🔇 Additional comments (2)
.github/workflows/build_images.yaml (2)
58-58: LGTM! Step name accurately reflects expanded functionality.The updated step name clearly indicates that commit SHA, commit time, and branch are all being captured.
61-61: LGTM! Commit time captured correctly.Using
%ctformat captures the commit timestamp as a Unix epoch time, which is suitable for machine processing and avoids timezone ambiguities.
| run: | | ||
| git rev-parse HEAD > ./ci/docker/context/COMMIT_SHA | ||
| git log -n1 --pretty='%ct' > ./ci/docker/context/COMMIT_TIME | ||
| git rev-parse --abbrev-ref HEAD > ./ci/docker/context/BRANCH |
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and read the build_images.yaml file
fd "build_images.yaml" --type fRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# List all files in .github/workflows directory
find .github/workflows -type f -name "*.yaml" -o -name "*.yml" 2>/dev/null | head -20Repository: NVIDIA/cuopt
Length of output: 505
🏁 Script executed:
#!/bin/bash
# Read the build_images.yaml file with line numbers, focusing on the relevant sections
cat -n .github/workflows/build_images.yaml | head -80Repository: NVIDIA/cuopt
Length of output: 3036
🏁 Script executed:
#!/bin/bash
# Check if there are any other branch-related operations or context that affects BRANCH usage
grep -n "BRANCH" .github/workflows/build_images.yamlRepository: NVIDIA/cuopt
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Check if there are other workflows that call this workflow and potentially pass branch info
grep -r "build_images.yaml" .github/workflows/ --include="*.yaml" --include="*.yml"Repository: NVIDIA/cuopt
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Check how build_images.yaml is called from the parent workflow
grep -A 10 "build_images.yaml" .github/workflows/build_test_publish_images.yamlRepository: NVIDIA/cuopt
Length of output: 473
Critical: git rev-parse --abbrev-ref HEAD will always capture "HEAD" instead of the actual branch name
Line 62 executes this command after checking out a specific commit SHA (line 47), which puts the repository in a detached HEAD state. In this state, the command always returns HEAD when you're in detached HEAD state, making the BRANCH file incorrect.
Recommended fix: Pass the branch name as an additional workflow input parameter and use it directly:
- git rev-parse --abbrev-ref HEAD > ./ci/docker/context/BRANCH
+ echo "${{ inputs.branch_name }}" > ./ci/docker/context/BRANCHAdd branch_name to the workflow inputs (around line 8-30) and have the caller pass it.
Alternative: Use git branch -r --contains HEAD | grep -oP 'origin/\K.*' | head -n1 if you need to derive the branch from the commit, though this may return multiple branches if the commit exists on several.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/build_images.yaml around line 62: the current command writes
the branch as `git rev-parse --abbrev-ref HEAD` but because the checkout uses a
specific commit SHA (detached HEAD) this will always write "HEAD"; change the
workflow to accept a branch_name input and use that input when creating
./ci/docker/context/BRANCH (add branch_name to the workflow inputs and ensure
callers pass it), or if you must derive the branch from the commit, replace the
current command with a remote-branch lookup such as using git branch -r
--contains HEAD piped to extract the origin/<branch> name and take the first
result (handle possible multiple matches).
Description
Issue
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.