Skip to content

[code-simplifier] refactor: extract lookupContainerPin helper to eliminate duplication (#27762 follow-up) - #27784

Merged
pelikhan merged 1 commit into
mainfrom
code-simplifier/extract-lookup-container-pin-helper-73a7030d4a71779b
Apr 22, 2026
Merged

[code-simplifier] refactor: extract lookupContainerPin helper to eliminate duplication (#27762 follow-up)#27784
pelikhan merged 1 commit into
mainfrom
code-simplifier/extract-lookup-container-pin-helper-73a7030d4a71779b

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Overview

Follow-up simplification to #27762 ("Pin builtin container images by digest"). The cache-first, embedded-fallback pattern for container pin resolution was introduced in two places with identical structure but different return fields used by each caller.

Files Simplified

  • pkg/workflow/action_pins.go — added lookupContainerPin helper
  • pkg/workflow/docker.goapplyContainerPins uses lookupContainerPin
  • pkg/workflow/awf_helpers.goresolveContainerDigest uses lookupContainerPin

Improvement Made

Reduced Duplication — Extracted lookupContainerPin helper

Both applyContainerPins (docker.go) and resolveContainerDigest (awf_helpers.go) implemented the same cache-first, embedded-fallback logic:

// Before — repeated in two places:ifcache!=nil {
ifpin, ok:=cache.GetContainerPin(img); ok&&pin.X!="" {
// use pincontinue
}
}
ifembeddedPin, ok:=getEmbeddedContainerPin(img); ok&&embeddedPin.X!="" {
// use embeddedPincontinue
}
// After — single shared lookup:ifpin, ok:=lookupContainerPin(img, cache); ok&&pin.X!="" {
// use pin
}

The new helper in action_pins.go:

funclookupContainerPin(imagestring, cache*ActionCache) (ContainerPin, bool) {
ifcache!=nil {
ifpin, ok:=cache.GetContainerPin(image); ok {
returnpin, true
}
}
ifpin, ok:=getEmbeddedContainerPin(image); ok {
returnContainerPin(pin), true
}
returnContainerPin{}, false
}

Changes Based On

Recent changes from:

Testing

  • ✅ All related tests pass (TestApplyContainerPins, TestBuildAWFImageTagWithDigests, TestBuildAWFArgs_ImageTagIncludesDigests, TestGetContainerPin_ReturnsPinnedImage)
  • go vet clean
  • ✅ Build succeeds (make build)
  • ✅ No functional changes — behavior is identical

Review Focus

Please verify:

  • Functionality is preserved (same cache-first, embedded-fallback semantics)
  • lookupContainerPin correctly unifies both lookup patterns

Automated by Code Simplifier Agent — analyzing code from the last 24 hours

Generated by Code Simplifier · ● 2.6M ·

  • expires on Apr 23, 2026, 6:24 AM UTC

The cache-first, embedded-fallback pattern for container pin resolution
was duplicated between applyContainerPins (docker.go) and
resolveContainerDigest (awf_helpers.go).
Extract lookupContainerPin into action_pins.go and use it in both
callers. This reduces each call site from 4-6 lines to a single
conditional, and ensures both code paths share the same lookup logic.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

Copy link
Copy Markdown
ContributorAuthor

Hey @github-actions[bot] 👋 — great clean-up on the lookupContainerPin extraction! The before/after breakdown in the description makes the intent crystal clear, and the reduction from ~18 lines of duplicated cache-first/embedded-fallback logic down to a single shared call is a solid simplification of the #27762 follow-up.

One thing that would make this even more solid:

  • Add a unit test for lookupContainerPin — the helper itself isn't directly exercised by the existing tests (which go through applyContainerPins and resolveContainerDigest). A small table-driven test covering the three cases — cache hit, embedded fallback, and not-found — would give the helper its own coverage and guard against future regressions independently of its callers.

If you'd like a hand, you can assign this prompt to your coding agent:

Add a unit test for the lookupContainerPin helper in pkg/workflow/action_pins.go (or the corresponding _test.go file).
Cover these three scenarios using a table-driven test:
1. Cache hit — cache contains the image pin → should return the cached ContainerPin and true.
2. Embedded fallback — cache is nil (or doesn't contain the image) but embedded pins has it → should return the embedded ContainerPin and true.
3. Not found — neither cache nor embedded pins has the image → should return an empty ContainerPin and false.
Follow the existing table-driven test patterns in pkg/workflow/ and run `make test-unit` to confirm all tests pass.

Generated by Contribution Check · ● 1.4M ·

@pelikhan
pelikhan marked this pull request as ready for review April 22, 2026 13:25
CopilotAI review requested due to automatic review settings April 22, 2026 13:25
@github-actionsgithub-actionsBot mentioned this pull request Apr 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors container pin resolution by extracting a shared lookupContainerPin helper to remove duplicated “cache-first, embedded-fallback” logic introduced in #27762.

Changes:

  • Added lookupContainerPin(image, cache) helper in pkg/workflow/action_pins.go.
  • Updated applyContainerPins to use the helper for pinned image resolution.
  • Updated resolveContainerDigest to use the helper for digest resolution.
Show a summary per file
FileDescription
pkg/workflow/action_pins.goIntroduces shared helper for container pin lookup (cache then embedded).
pkg/workflow/docker.goUses helper inside applyContainerPins to resolve pinned image refs and manifest entries.
pkg/workflow/awf_helpers.goUses helper inside resolveContainerDigest for AWF image-tag digest metadata.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment on lines 207 to 212
for i, img := range images {
if cache != nil {
if pin, ok := cache.GetContainerPin(img); ok && pin.PinnedImage != "" {
result[i] = pin.PinnedImage
pins[i] = GHAWManifestContainer(pin)
dockerLog.Printf("Pinned container image: %s -> %s", img, pin.PinnedImage)
continue
}
}
if embeddedPin, ok := getEmbeddedContainerPin(img); ok && embeddedPin.PinnedImage != "" {
result[i] = embeddedPin.PinnedImage
pins[i] = GHAWManifestContainer(embeddedPin)
dockerLog.Printf("Pinned container image from embedded pins: %s -> %s", img, embeddedPin.PinnedImage)
if pin, ok := lookupContainerPin(img, cache); ok && pin.PinnedImage != "" {
result[i] = pin.PinnedImage
pins[i] = GHAWManifestContainer(pin)
dockerLog.Printf("Pinned container image: %s -> %s", img, pin.PinnedImage)
continue
Comment on lines +85 to +95
// lookupContainerPin returns the ContainerPin for the given image, checking cache first
// then falling back to embedded pins. Returns false if the image is not pinned.
func lookupContainerPin(image string, cache *ActionCache) (ContainerPin, bool) {
if cache != nil {
if pin, ok := cache.GetContainerPin(image); ok {
return pin, true
}
}
if pin, ok := getEmbeddedContainerPin(image); ok {
return ContainerPin(pin), true
}
@pelikhan
pelikhan merged commit 1d778b7 into mainApr 22, 2026
92 of 94 checks passed
@pelikhan
pelikhan deleted the code-simplifier/extract-lookup-container-pin-helper-73a7030d4a71779b branch April 22, 2026 13:38
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pelikhan