fix(tracing-versions): stamp service versions for tracing#383
Conversation
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
📝 WalkthroughWalkthroughThe workspace status scripts now resolve service-specific exact tags with semantic version sorting, while preserving explicit and fallback version handling. The manual image-push workflow passes its computed snapshot tag through ChangesVersion resolution and image publishing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/image-push-manual.ymlTraceback (most recent call last): Comment |
08c4ed3 to
9e3ef4b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/control-plane-services/function-autoscaler/tools/workspace_status.sh`:
- Around line 33-42: Update version_from_exact_tag in both
src/control-plane-services/function-autoscaler/tools/workspace_status.sh (lines
33-42) and
src/invocation-plane-services/http-invocation/tools/workspace_status.sh (lines
33-42) to evaluate matching tags across all prefixes before selecting the
highest semantic version, or explicitly enforce canonical-prefix precedence. Add
regression coverage for cases where canonical and legacy prefixes both match,
verifying identical correct resolution in both scripts.
In `@src/invocation-plane-services/http-invocation/tools/workspace_status.sh`:
- Around line 33-48: Add repository-native Bazel regression coverage for the
version-selection contract in workspace_status.sh, covering canonical-only tags,
legacy-only tags, both prefixes, no-tag fallback, and NVCF_VERSION propagation
into the stamped Bazel invocation. Place the checks in the appropriate subtree
test target and run the repository-native Bazel validation for
src/invocation-plane-services/http-invocation/**/*.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00050e5a-e605-41ee-baa4-7ea0007e1b89
📒 Files selected for processing (3)
.github/workflows/image-push-manual.ymlsrc/control-plane-services/function-autoscaler/tools/workspace_status.shsrc/invocation-plane-services/http-invocation/tools/workspace_status.sh
| version_from_exact_tag() { | ||
| local prefix tag | ||
| for prefix in "$@"; do | ||
| tag=$(git tag --points-at HEAD --list "${prefix}*" 2>/dev/null | sort -V | tail -n1 || true) | ||
| if [ -n "${tag}" ]; then | ||
| printf '%s\n' "${tag#"${prefix}"}" | ||
| return 0 | ||
| fi | ||
| done | ||
| return 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix cross-prefix version selection in both workspace scripts.
Both helpers return after the first prefix match, so they do not reliably select the highest matching service tag when canonical and legacy tags coexist.
src/control-plane-services/function-autoscaler/tools/workspace_status.sh#L33-L42: collect suffixes from both prefixes before semantic sorting, or explicitly enforce canonical-prefix precedence.src/invocation-plane-services/http-invocation/tools/workspace_status.sh#L33-L42: apply the same fix and add regression coverage for both-prefix resolution.
📍 Affects 2 files
src/control-plane-services/function-autoscaler/tools/workspace_status.sh#L33-L42(this comment)src/invocation-plane-services/http-invocation/tools/workspace_status.sh#L33-L42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/control-plane-services/function-autoscaler/tools/workspace_status.sh`
around lines 33 - 42, Update version_from_exact_tag in both
src/control-plane-services/function-autoscaler/tools/workspace_status.sh (lines
33-42) and
src/invocation-plane-services/http-invocation/tools/workspace_status.sh (lines
33-42) to evaluate matching tags across all prefixes before selecting the
highest semantic version, or explicitly enforce canonical-prefix precedence. Add
regression coverage for cases where canonical and legacy prefixes both match,
verifying identical correct resolution in both scripts.
| version_from_exact_tag() { | ||
| local prefix tag | ||
| for prefix in "$@"; do | ||
| tag=$(git tag --points-at HEAD --list "${prefix}*" 2>/dev/null | sort -V | tail -n1 || true) | ||
| if [ -n "${tag}" ]; then | ||
| printf '%s\n' "${tag#"${prefix}"}" | ||
| return 0 | ||
| fi | ||
| done | ||
| return 1 | ||
| } | ||
|
|
||
| if [ -n "${NVCF_VERSION:-}" ]; then | ||
| VERSION="${NVCF_VERSION}" | ||
| elif TAG=$(git describe --tags --exact-match HEAD 2>/dev/null); then | ||
| VERSION="${TAG}" | ||
| elif VERSION=$(version_from_exact_tag "${TAG_PREFIX}" "${LEGACY_TAG_PREFIX}"); then | ||
| : |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add repository-native regression coverage for the version contract.
The stated validation covers shell syntax and diff formatting, but not canonical-only, legacy-only, both-prefix, no-tag fallback, or NVCF_VERSION propagation into the stamped Bazel invocation. Add these checks or explain why tests are not applicable; use Bazel validation for this subtree.
As per coding guidelines, code changes must include tests or explain why tests are not applicable, run the repository-native test runner, and use Bazel as the canonical validation path for src/invocation-plane-services/http-invocation/**/*.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/invocation-plane-services/http-invocation/tools/workspace_status.sh`
around lines 33 - 48, Add repository-native Bazel regression coverage for the
version-selection contract in workspace_status.sh, covering canonical-only tags,
legacy-only tags, both prefixes, no-tag fallback, and NVCF_VERSION propagation
into the stamped Bazel invocation. Place the checks in the appropriate subtree
test target and run the repository-native Bazel validation for
src/invocation-plane-services/http-invocation/**/*.
Source: Coding guidelines
9e3ef4b to
82f9d0a
Compare
1 participant
TL;DR
Stamp the real release version into the HTTP invocation and function autoscaler Bazel image builds so tracing reports the service-specific version instead of stale package defaults or unrelated monorepo tags.
Additional Details
NVCF_VERSIONfrom the manual image push workflow and run the Bazel image push with--stamp.workspace_status.shscripts to preferNVCF_VERSION.For the Reviewer
Please look closely at:
.github/workflows/image-push-manual.ymlsrc/invocation-plane-services/http-invocation/tools/workspace_status.shsrc/control-plane-services/function-autoscaler/tools/workspace_status.shFor QA
Verified with:
bash -n src/control-plane-services/function-autoscaler/tools/workspace_status.shbash -n src/invocation-plane-services/http-invocation/tools/workspace_status.shgit diff --check origin/main...HEADQA needed: no separate QA pass expected; this is build/stamping behavior.
Issues
NO-REF
Checklist
Summary by CodeRabbit