shcizo/package-updater
1
0
Fork 0
Code Issues 8 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(master): review fixes for newer-tag suggestions (#11)

Browse Source
This commit is contained in:
shcizo
2026-10-13 11:56:03 +02:00
parent c9cb1103ad
commit a5d0d1a070
11 changed files with 111 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CLAUDE.md
+3 -1
View File
@@ -83,7 +83,9 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/
- **`services.newer_ref` is a suggestion only**: no auto jobs, not even for auto services. The
poller writes it only on change; a failed tag list keeps the old value and never sets
`last_poll_error`. The UI re-checks it at render time with `tagver.Newer`, so a stale value
is hidden until the next cycle. Tags with one numeric component or no digits (`7`, `pg16`,
is hidden until the next cycle; it is also hidden when its repo is not the one the service
follows. The suggestion is computed from the service list read at the start of the cycle, so
if the service moved on meanwhile, the render-time check hides it. Tags with one numeric component or no digits (`7`, `pg16`,
`latest`) never get suggestions.
- **`image_versions` uses `''` instead of NULL** for a missing tag or digest, so
`UNIQUE (repo, tag, digest)` deduplicates. The webhook records `(repo, tag, '')`; that
internal/master/poller/poller.go
+43 -24
View File
@@ -164,16 +164,26 @@ func (p *Poller) Cycle(ctx context.Context) {
}
p.record(write, l, byRef[l.ref], &stats)
}
// Publish after the digest phase too: listing tags can take a while and
// an "Update available" should not wait for it.
p.publishIfWrote(&stats)
p.syncNewer(ctx, write, svcs, &stats)
if stats.wrote {
p.pub.Publish(events.Services)
}
p.publishIfWrote(&stats)
p.log.Info("poll_cycle", "refs", stats.refs, "ok", stats.ok, "failed", stats.failed,
"suggested", stats.suggested, "jobs", stats.jobs, "skipped", stats.skipped,
"repos_listed", stats.reposListed, "tags_failed", stats.tagsFailed, "newer", stats.newer,
"duration_ms", p.now().Sub(began).Milliseconds())
}
// publishIfWrote tells the browsers to refresh if a phase of the cycle wrote
// anything, and starts the next phase with a clean slate.
func (p *Poller) publishIfWrote(stats *cycleStats) {
if stats.wrote {
p.pub.Publish(events.Services)
stats.wrote = false
}
}
// forEach calls fn(i) for i in [0, n), Config.Workers at a time, and stops
// starting new calls once ctx is done. It returns when all started calls
// have returned.
@@ -246,27 +256,35 @@ func (p *Poller) listAll(ctx context.Context, repos []string) []tagList {
return out
}
// followedTag returns the repo and tag svc follows. ok is false unless svc is
// polled and the tag can have a newer tag.
func followedTag(svc model.Service) (repo, tag string, ok bool) {
// Followed returns the repo and tag svc follows: a bare image follows
// "latest", and a digest-only reference follows no tag.
func Followed(svc model.Service) (repo, tag string) {
ref := imageref.Parse(jobs.FollowedRef(svc.ActualRef))
return ref.Repo, ref.Tag
}
// followedComparable returns the repo and tag svc follows. ok is false unless
// svc is polled and the tag can have a newer tag.
func followedComparable(svc model.Service) (repo, tag string, ok bool) {
if SkipReason(svc) != "" {
return "", "", false
}
ref := imageref.Parse(jobs.FollowedRef(svc.ActualRef))
return ref.Repo, ref.Tag, tagver.Comparable(ref.Tag)
repo, tag = Followed(svc)
return repo, tag, tagver.Comparable(tag)
}
// syncNewer lists the tags of every repo that has a service following a
// version-like tag, once per repo, and stores the newer tag of each service.
// A service whose tag cannot have a newer tag any more gets its suggestion
// cleared; one whose repo could not be listed keeps it. Nothing is written
// cleared; one whose repo could not be listed keeps it, unless it names
// another repo than the service follows now. Nothing is written
// when the value is unchanged. It never starts a job, not even for an auto
// service: a newer tag may be a major step the user has not chosen.
func (p *Poller) syncNewer(ctx, write context.Context, svcs []model.Service, stats *cycleStats) {
seen := map[string]bool{}
var repos []string
for _, svc := range svcs {
if repo, _, ok := followedTag(svc); ok && svc.RemovedAt == nil && !seen[repo] {
if repo, _, ok := followedComparable(svc); ok && svc.RemovedAt == nil && !seen[repo] {
seen[repo] = true
repos = append(repos, repo)
}
@@ -274,11 +292,10 @@ func (p *Poller) syncNewer(ctx, write context.Context, svcs []model.Service, sta
lists := map[string][]string{}
for _, l := range p.listAll(ctx, repos) {
if l.err != nil {
if ctx.Err() != nil {
continue // cancelled mid-listing: not the registry's fault
}
stats.tagsFailed++
p.log.Warn("poll_tags_failed", "repo", l.repo, "error", truncate(l.err.Error()))
if ctx.Err() == nil { // cancelled mid-listing: not the registry's fault
stats.tagsFailed++
}
continue
}
stats.reposListed++
@@ -292,22 +309,24 @@ func (p *Poller) syncNewer(ctx, write context.Context, svcs []model.Service, sta
continue
}
want := ""
if repo, tag, ok := followedTag(svc); ok {
if repo, tag, ok := followedComparable(svc); ok {
tags, listed := lists[repo]
if !listed {
if !listed && imageref.Parse(svc.NewerRef).Repo == repo {
if svc.NewerRef != "" {
stats.newer++
}
continue // not listed this cycle: keep what is stored
}
key := repoTag{repo, tag}
n, done := newest[key]
if !done {
n, _ = tagver.Newer(tag, tags)
newest[key] = n
}
if n != "" {
want = repo + ":" + n
if listed {
key := repoTag{repo, tag}
n, done := newest[key]
if !done {
n, _ = tagver.Newer(tag, tags)
newest[key] = n
}
if n != "" {
want = repo + ":" + n
}
}
}
if want != "" {
internal/master/poller/poller_test.go
+32
View File
@@ -888,3 +888,35 @@ func TestCancelledListKeepsNewer(t *testing.T) {
assert.Empty(t, st.newer)
assert.Empty(t, pub.topics)
}
func TestFailedListClearsNewerFromAnotherRepo(t *testing.T) {
s := newerSvc(1, repoB, "1.27.3")
s.NewerRef = repoA + ":1.27.4"
st := &fakeStore{services: []model.Service{s}}
reg := newerRegistry(nil, repoB+":1.27.3")
reg.tagErrs = map[string]error{repoB: errors.New("list x: registry answered 500 Internal Server Error")}
p, _ := newPoller(st, &fakeJobs{}, reg, Config{})
p.Cycle(context.Background())
assert.Equal(t, []newerCall{{1, "", time.Date(2026, 10, 13, 12, 0, 0, 0, time.UTC)}}, st.newer)
}
func TestDigestWritesArePublishedBeforeTagsAreListed(t *testing.T) {
st := &fakeStore{services: []model.Service{svc(1, repoB+":1.27.3")}}
reg := newerRegistry(map[string][]string{repoB: {"1.27.3"}}, repoB+":1.27.3")
reg.tagRelease = make(chan struct{})
reg.tagStarted = make(chan struct{}, 1)
p, pub := newPoller(st, &fakeJobs{}, reg, Config{})
done := make(chan struct{})
go func() { p.Cycle(context.Background()); close(done) }()
<-reg.tagStarted
pub.mu.Lock()
published := append([]events.Topic(nil), pub.topics...)
pub.mu.Unlock()
assert.Equal(t, []events.Topic{events.Services}, published, "published while the listing is still blocked")
close(reg.tagRelease)
<-done
}
internal/master/registry/registry.go
+1 -1
View File
@@ -103,7 +103,7 @@ func (c *Client) lookupError(verb, target, ref string, err error) error {
if errors.As(err, &terr) {
return fmt.Errorf("%s %s: %s", verb, target, describe(terr))
}
c.log.Warn("registry_lookup_failed", "ref", ref, "error", err)
c.log.Warn("registry_request_failed", "ref", ref, "error", err)
return fmt.Errorf("%s %s: %s", verb, target, classify(err))
}
internal/master/store/merge.go
+4 -3
View File
@@ -107,8 +107,8 @@ type mergeRow struct {
// MergeService moves the live service toID onto the removed service fromID:
// the old row keeps its id, first_seen_at, update mode and desired ref, takes
// the location and inventory of the live row (and its available and newer-tag
// suggestions), becomes live, and the live row is deleted. The jobs of the live row move to
// the old one. This is the only place a services row is deleted.
// suggestions), becomes live, and the live row is deleted. The jobs of the live
// row move to the old one. This is the only place a services row is deleted.
//
// It returns *model.MergeRefusedError when the pair cannot be merged,
// model.ErrJobActive when either row has a queued or running job,
@@ -213,8 +213,9 @@ func (s *Store) MergeService(ctx context.Context, fromID, toID int64) error {
config_files = $7, built = $8, swarm_cluster_id = $9, swarm_service = $10, container_name = $11,
image_repo = $12, actual_ref = $13, actual_digest = $14, status = $15, self = $16, updated_at = $17,
available_ref = $18, available_digest = $19, available_source = $20, available_at = $21,
last_poll_error = $22, newer_ref = $24, newer_at = $25,
last_poll_error = $22,
last_tag_digest = CASE WHEN last_tag_digest <> '' THEN last_tag_digest ELSE $23 END,
newer_ref = $24, newer_at = $25,
removed_at = NULL, awaiting_inventory = false
WHERE id = $1`,
fromID, t.AgentID, t.LocationKey, t.ComposeProject, t.ComposeService, t.WorkingDir,
internal/master/store/services.go
+1 -1
View File
@@ -340,7 +340,7 @@ func (s *Store) GetImageVersion(ctx context.Context, id int64) (model.ImageVersi
return v, nil
}
// GetTagVersion returns the digest-less version row with an empty digest, which
// GetTagVersion returns the version row of repo and tag with an empty digest, which
// SetNewer and the webhook create. Returns model.ErrNotFound if there is none.
func (s *Store) GetTagVersion(ctx context.Context, repo, tag string) (model.ImageVersion, error) {
var v model.ImageVersion
internal/master/web/server.go
+1 -1
View File
@@ -393,7 +393,7 @@ func (s *Server) serviceDetail(r *http.Request, id int64) (pageData, error) {
return data, err
}
data.NewerTag = newerTag(svc)
if data.NewerTag != "" && !slices.ContainsFunc(versions, func(v model.ImageVersion) bool { return v.Tag == data.NewerTag }) {
if data.NewerTag != "" && !slices.ContainsFunc(versions, func(v model.ImageVersion) bool { return v.Tag == data.NewerTag && v.Digest == "" }) {
// The suggested tag can be older than the listed versions; it must
// still be pickable.
v, err := s.store.GetTagVersion(r.Context(), svc.ImageRepo, data.NewerTag)
internal/master/web/static/app.css
+1 -2
View File
@@ -106,10 +106,9 @@ summary.btn::-webkit-details-marker{display:none}
.tag em{font:500 11.5px var(--mono);font-style:normal;margin-left:6px}
.tag em:empty{display:none}
.f-up{color:var(--orange);background:var(--orangebg)}
.f-newer{color:var(--mut);box-shadow:inset 0 0 0 1px var(--line2)}
.f-newer em{color:var(--orange)}
.f-err{color:var(--red);background:var(--redbg)}
.f-notag{color:var(--mut);box-shadow:inset 0 0 0 1px var(--line2)}
.f-newer,.f-notag{color:var(--mut);box-shadow:inset 0 0 0 1px var(--line2)}
.f-out{color:var(--dim);box-shadow:inset 0 0 0 1px var(--line)}
.mode{display:flex;flex-direction:column;align-items:flex-start;gap:2px}
.mode form{margin:0;display:flex;flex-direction:column;align-items:flex-start;gap:2px}
internal/master/web/versions_test.go
+15
View File
@@ -247,6 +247,21 @@ func TestServicePage_NewerTagAlreadyListedIsNotAppendedTwice(t *testing.T) {
assert.Equal(t, 1, strings.Count(body, `name="version"`))
}
func TestServicePage_PinnedRowOfTheNewerTagDoesNotHideThePlainRow(t *testing.T) {
srv, store, _, _, _ := newTestEnv(t)
store.services[0].ImageRepo = sluskRepo
store.services[0].NewerRef = sluskRepo + ":1.5.0"
store.versions = []model.ImageVersion{
{ID: 31, Repo: sluskRepo, Tag: "1.5.0", Digest: "sha256:d", Source: "poll"},
{ID: 32, Repo: sluskRepo, Tag: "1.5.0", Source: "tags"},
}
_, body := get(t, srv, "/services/1")
assert.Equal(t, 2, strings.Count(body, `name="version"`))
assert.Contains(t, body, `<input type="hidden" name="version" value="32">`)
}
func TestServicePage_NoNewerTagRowWithoutSuggestion(t *testing.T) {
srv, _, _, _, _ := newTestEnv(t)
internal/master/web/views.go
+9 -5
View File
@@ -249,17 +249,21 @@ func followsNoTag(svc model.Service) bool {
// newerTag returns the tag of the service's newer-tag suggestion, or "" when
// there is none or it no longer holds. The poller writes the suggestion once
// per cycle, so after the service moved to another tag the stored value can
// be stale until the next cycle; checking again here hides it meanwhile.
// be stale until the next cycle; checking again here hides it meanwhile. A
// suggestion from another repo than the service follows now is hidden too.
func newerTag(s model.Service) string {
if s.RemovedAt != nil || s.NewerRef == "" {
return ""
}
tag := imageref.Parse(s.NewerRef).Tag
followed := imageref.Parse(jobs.FollowedRef(s.ActualRef)).Tag
if _, ok := tagver.Newer(followed, []string{tag}); !ok {
newer := imageref.Parse(s.NewerRef)
repo, followed := poller.Followed(s)
if newer.Repo != repo {
return ""
}
return tag
if _, ok := tagver.Newer(followed, []string{newer.Tag}); !ok {
return ""
}
return newer.Tag
}
// versionRow is an image version as shown on the service page.
internal/master/web/views_test.go
+1
View File
@@ -223,6 +223,7 @@ func TestNewerTag(t *testing.T) {
{"bare image follows latest", model.Service{ActualRef: "gitea/gitea", NewerRef: "gitea/gitea:1.28.0"}, ""},
{"stale after the tag changed", model.Service{ActualRef: "gitea/gitea:1.28.0", NewerRef: "gitea/gitea:1.28.0"}, ""},
{"stale after the tag moved past it", model.Service{ActualRef: "gitea/gitea:1.29.0", NewerRef: "gitea/gitea:1.28.0"}, ""},
{"other repo", model.Service{ActualRef: "redis:7.2.4", NewerRef: "bitnami/redis:7.2.5"}, ""},
{"removed", model.Service{ActualRef: "gitea/gitea:1.27.3", NewerRef: "gitea/gitea:1.28.0", RemovedAt: &removed}, ""},
{"empty", model.Service{ActualRef: "gitea/gitea:1.27.3"}, ""},
}