fix: don't write empty tag when registry returns no tags#1505
fix: don't write empty tag when registry returns no tags#1505chengfang merged 3 commits intoargoproj-labs:masterfrom
Conversation
mark-liu
requested review from
chengfang,
dkarpele,
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughGetNewestVersionFromTags now returns nil (and logs a warning) when a registry returns no tags or when no suitable version matches; tests in image versioning and application update were adjusted/added to assert images are skipped in those cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Updater as Image Updater
participant Scanner as Registry Scanner
participant Version as GetNewestVersionFromTags
participant App as Application Processor
Updater->>Scanner: Request tags for image
Scanner-->>Version: Provide tag list (may be empty)
Version->>Version: evaluate tags against constraint
alt tags empty or no matching versions
Version-->>Scanner: return nil (and log warning)
Scanner-->>Updater: no newest version (nil)
Updater->>App: skip image update (increment NumSkipped)
else valid newest version found
Version-->>Scanner: return newest version
Scanner-->>Updater: newest version
Updater->>App: apply update (increment NumImagesUpdated)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1505 +/- ##
==========================================
+ Coverage 71.48% 73.51% +2.03%
==========================================
Files 50 53 +3
Lines 4667 5098 +431
==========================================
+ Hits 3336 3748 +412
- Misses 1133 1143 +10
- Partials 198 207 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
registry-scanner/pkg/image/version.go (1)
81-83: Stale function comment — update to reflect nil-return semanticsThe docstring says "Returns the original version if no new version could be found from the list of tags." After this PR, the function now returns
nilin that situation, not the original image tag.✏️ Suggested docstring update
// GetNewestVersionFromTags returns the latest available version from a list of // tags while optionally taking a semver constraint into account. Returns the -// original version if no new version could be found from the list of tags. +// nil version if no new version could be found from the list of tags, or if +// the registry returned no tags at all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry-scanner/pkg/image/version.go` around lines 81 - 83, The docstring for GetNewestVersionFromTags is stale: it currently says "Returns the original version if no new version could be found," but the function now returns nil in that case; update the comment for GetNewestVersionFromTags to accurately state that it returns a pointer to the newest version matching the (optional) semver constraint or nil when no newer version is found (and preserve any existing mention of error or constraint behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@registry-scanner/pkg/image/version.go`:
- Around line 81-83: The docstring for GetNewestVersionFromTags is stale: it
currently says "Returns the original version if no new version could be found,"
but the function now returns nil in that case; update the comment for
GetNewestVersionFromTags to accurately state that it returns a pointer to the
newest version matching the (optional) semver constraint or nil when no newer
version is found (and preserve any existing mention of error or constraint
behavior).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
registry-scanner/pkg/image/version.go (1)
149-157: Silentnilreturn when tags exist but none pass constraint/strategy filtering.Line 149 logs at
Debuglevel ("found 0 from N tags eligible for consideration"), but there is noWarn-level message whenconsiderTagsis empty. This creates an observability gap: users whose constraint or ignore-list filters out every tag get no actionable signal at the default log level, while the empty-registry path (line 103) now does warn.Consider adding a warning here for parity:
💡 Proposed addition
if len(considerTags) > 0 { return considerTags[len(considerTags)-1], nil } + logCtx.Warnf("no tags for image %s matched the current constraint/strategy (checked %d tags)", img.GetFullNameWithoutTag(), len(availableTags)) return nil, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry-scanner/pkg/image/version.go` around lines 149 - 157, The function in registry-scanner/pkg/image/version.go currently logs at Debug when no tags pass constraint/strategy filtering (logCtx.Debugf with considerTags length), but then silently returns nil; add a Warn-level log (using logCtx.Warnf or logCtx.Warn) in the branch where len(considerTags) == 0 AND len(availableTags) > 0 to surface that all candidate tags were filtered out by constraints/ignore rules; reference the existing logCtx usage and the return path that currently returns nil so the warning is emitted before the final return to provide parity with the empty-registry warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@registry-scanner/pkg/image/version.go`:
- Around line 102-105: The warning misattributes an empty availableTags to the
registry when tags may have been filtered out by SortBySemVer; modify the logic
around SortBySemVer/availableTags so you check the pre-filter tag count (e.g.,
the original tags slice returned from the registry) and update the log via
logCtx.Warnf to distinguish "no tags in registry" vs "registry returned X tags
but none matched semver (filtered by SortBySemVer) for image %s" using
img.GetFullNameWithoutTag() to provide context.
---
Nitpick comments:
In `@registry-scanner/pkg/image/version.go`:
- Around line 149-157: The function in registry-scanner/pkg/image/version.go
currently logs at Debug when no tags pass constraint/strategy filtering
(logCtx.Debugf with considerTags length), but then silently returns nil; add a
Warn-level log (using logCtx.Warnf or logCtx.Warn) in the branch where
len(considerTags) == 0 AND len(availableTags) > 0 to surface that all candidate
tags were filtered out by constraints/ignore rules; reference the existing
logCtx usage and the return path that currently returns nil so the warning is
emitted before the final return to provide parity with the empty-registry
warning.
dkarpele
changed the title
|
Friendly ping — this fixes a data loss edge case where an empty tag gets written. Would appreciate a review when you get a chance. |
|
Hello @mark-liu , I've reported a similar bug, but more linked to a pod scale-down (rather than a registry giving no tag), resulting an empty image tag, do you know by any chance if your implementation will solve this too? Thanks |
|
Hey @Resousse, thanks for linking #1523. I suspect so. When the pod scales to zero, ArgoCD's Status.Summary.Images either goes empty or retains the stale image ref. In both cases the path through
There's also a defence-in-depth guard in SetHelmImage that skips setting the tag parameter when GetTagWithDigest() is empty, so even if something slips through, the empty value shouldn't reach your values file. Not sure if you can, but could you try this branch against your setup and confirm it stops the loop? |
|
@mark-liu I've tested your branch, and unfortunately, it doesn't solve my issue. As soon as, the scale down is done, an empty tag "" is write back in git causing the loop again :/ |
|
Ok, I found the issue, in file I've tested this based on your branch, and it works as expected @mark-liu do you mind adding this code to your PR, as it's related to empty tag ? |
|
Thanks for the investigation @Resousse. You've found a separate code path — this PR guards the "registry returns no tags" entry point ( Your proposed guard looks good to me: if tagValue != "" {
err = setHelmValue(&helmNewValues, helmParamVersion, tagValue)
}When the file already exists, Worth noting: the ArgoCD write-back path already has this guard at Since this is a different trigger and code path from #1242, it should be a separate PR linked to #1523. Would you like to open one? The fix is a one-line guard at |
|
Sure, just created this : #1531 |
Signed-off-by: Mark Liu <mark@prove.com.au>
Signed-off-by: Mark Liu <mark@prove.com.au>
Distinguish between registry returning no tags vs strategy/constraint filtering all tags out, so operators can diagnose the root cause. Signed-off-by: Mark Liu <mark@prove.com.au>
chengfang
force-pushed
the
fix/empty-registry-tags
branch
from
554284b to
83c6603
Compare
4 participants
Fixes #1242
When a registry returns zero tags (happened during Bitnami's SemVer → Digest migration),
GetNewestVersionFromTagsreturned the currentImageTagwhich could be a dummy with emptyTagName. This bypassed the nil guard inUpdateApplicationand wroteimage.tag: ""to the manifest.Fix is one line — return
nilinstead ofimg.ImageTagwhen no tags are available. The existing skip logic inUpdateApplicationalready handles nil correctly.latest,alpine) — these were silently treated as "up-to-date", now correctly classified as "skipped" with a warning log. No update happened either way, but users may see newWARN no tags foundmessages for misconfigured images.Related
This PR fixes the "registry returns no tags" path into empty tag write-back. A separate code path exists in
marshalParamsOverride(update.go:~438) whereGetTagWithDigest()returns""when a pod scales to zero — reported in #1523 by @Resousse. That requires a separate guard and should be addressed in its own PR.Summary by CodeRabbit