Repository: project-copacetic/copacetic
Pull request #1098 (2025-06-06 22:21:14)

feat: Add Multi Arch Enhancements
Added table summary for Multi-Arch Patching with Platform, Status, Ref, Error as fields. Wanted feedback on how can I test this code. I'm confused on how to simulate multi-arch patching locally using docker and trivy to test the credibility and making sure the logs are as expected.
Please review the code and let me know this is how we wanted the logs to look. Thanks!

closes: #992


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/project-copacetic/copacetic/pull/1098?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=project-copacetic) Report
Attention: Patch coverage is `0%` with `50 lines` in your changes missing coverage. Please review.
> Project coverage is 43.08%. Comparing base [(`c8c5df7`)](https://app.codecov.io/gh/project-copacetic/copacetic/commit/c8c5df708108de8f323f5705de6d21ba2fcbda80?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=project-copacetic) to head [(`3aee9af`)](https://app.codecov.io/gh/project-copacetic/copacetic/commit/3aee9af3b3bb462523104251d155d1762afd8d97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=project-copacetic).

| [Files with missing lines](https://app.codecov.io/gh/project-copacetic/copacetic/pull/1098?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=project-copacetic) | Patch % | Lines |
|---|---|---|
| [pkg/patch/patch.go](https://app.codecov.io/gh/project-copacetic/copacetic/pull/1098?src=pr&el=tree&filepath=pkg%2Fpatch%2Fpatch.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=project-copacetic#diff-cGtnL3BhdGNoL3BhdGNoLmdv) | 0.00% | [50 Missing :warning: ](https://app.codecov.io/gh/project-copacetic/copacetic/pull/1098?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=project-copacetic) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1098      +/-   ##
==========================================
- Coverage   43.57%   43.08%   -0.49%     
==========================================
  Files          21       21              
  Lines        3282     3319      +37     
==========================================
  Hits         1430     1430              
- Misses       1741     1778      +37     
  Partials      111      111              
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/project-copacetic/copacetic/pull/1098?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=project-copacetic).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=project-copacetic).

<details><summary> :rocket: New features to boost your workflow: </summary>

- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
amanycodes: The improved UX logs are working, i tested the multi-arch patching by building alpine:3.17.9 for amd64, arm64 and arm/v7 even though it ran just for the amd64 the patchMultiArch Image was triggered and I got the tabular logs:
![image](https://github.com/user-attachments/assets/b8872628-f57e-4c9d-87f1-fe858a595dd4)

Opening this for review and ready for feedback!
cc: @robert-cronin 
amanycodes: @robert-cronin Thanks for the review! Please let me know if it looks fine now.

Patch:
diff --git a/pkg/buildkit/buildkit.go b/pkg/buildkit/buildkit.go
index 4b715d15..ccdcc673 100644
--- a/pkg/buildkit/buildkit.go
+++ b/pkg/buildkit/buildkit.go
@@ -209,6 +209,16 @@ func DiscoverPlatformsFromReference(manifestRef string) ([]types.PatchPlatform,
 	return nil, nil
 }
 
+//nolint:gocritic
+func PlatformKey(pl ispec.Platform) string {
+	// if platform is present in list from reference and report, then we should patch that platform
+	key := pl.OS + "/" + pl.Architecture
+	if pl.Variant != "" {
+		key += "/" + pl.Variant
+	}
+	return key
+}
+
 func DiscoverPlatforms(manifestRef, reportDir, scanner string) ([]types.PatchPlatform, error) {
 	var platforms []types.PatchPlatform
 
@@ -228,18 +238,13 @@ func DiscoverPlatforms(manifestRef, reportDir, scanner string) ([]types.PatchPla
 		}
 		log.WithField("platforms", p2).Debug("Discovered platforms from report")
 
-		// if platform is present in list from reference and report, then we should patch that platform
-		key := func(pl ispec.Platform) string {
-			return pl.OS + "/" + pl.Architecture + "/" + pl.Variant
-		}
-
 		reportSet := make(map[string]string, len(p2))
 		for _, pl := range p2 {
-			reportSet[key(pl.Platform)] = pl.ReportFile
+			reportSet[PlatformKey(pl.Platform)] = pl.ReportFile
 		}
 
 		for _, pl := range p {
-			if rp, ok := reportSet[key(pl.Platform)]; ok {
+			if rp, ok := reportSet[PlatformKey(pl.Platform)]; ok {
 				pl.ReportFile = rp
 				platforms = append(platforms, pl)
 			}
diff --git a/pkg/patch/patch.go b/pkg/patch/patch.go
index 7a8c633c..ede49ee0 100644
--- a/pkg/patch/patch.go
+++ b/pkg/patch/patch.go
@@ -12,6 +12,7 @@ import (
 	"runtime"
 	"strings"
 	"sync"
+	"text/tabwriter"
 	"time"
 
 	"github.com/containerd/platforms"
@@ -768,18 +769,12 @@ func patchMultiPlatformImage(
 	var mu sync.Mutex
 	patchResults := []types.PatchResult{}
 
-	handlePlatformErr := func(p types.PatchPlatform, err error) error {
-		if ignoreError {
-			log.Warnf("Ignoring error for platform %s: %v", p.OS+"/"+p.Architecture, err)
-			return nil
-		}
-		log.Warnf("Error for platform %s: %v", p.OS+"/"+p.Architecture, err)
-		return fmt.Errorf("platform %s failed: %w", p.OS+"/"+p.Architecture, err)
-	}
+	summaryMap := make(map[string]*types.MultiArchSummary)
 
 	for _, p := range platforms {
 		// rebind
 		p := p //nolint
+		platformKey := buildkit.PlatformKey(p.Platform)
 		g.Go(func() error {
 			select {
 			case sem <- struct{}{}:
@@ -789,15 +784,40 @@ func patchMultiPlatformImage(
 			defer func() { <-sem }()
 
 			res, err := patchSingleArchImage(gctx, ch, image, p.ReportFile, patchedTag, suffix, workingFolder, scanner, format, output, p, ignoreError, push, bkOpts, true)
+			mu.Lock()
+			defer mu.Unlock()
 			if err != nil {
-				return handlePlatformErr(p, err)
+				status := "Error"
+				if ignoreError {
+					status = "Ignored"
+				}
+				summaryMap[platformKey] = &types.MultiArchSummary{
+					Platform: platformKey,
+					Status:   status,
+					Ref:      "",
+					Error:    err.Error(),
+				}
+				if !ignoreError {
+					return err
+				}
+				return nil
 			} else if res == nil {
-				return fmt.Errorf("patchSingleArchImage returned nil result for platform %s", p.OS+"/"+p.Architecture)
+				summaryMap[platformKey] = &types.MultiArchSummary{
+					Platform: platformKey,
+					Status:   "Error",
+					Ref:      "",
+					Error:    "patchSingleArchImage returned nil result",
+				}
+				return nil
 			}
 
-			mu.Lock()
 			patchResults = append(patchResults, *res)
-			mu.Unlock()
+			summaryMap[platformKey] = &types.MultiArchSummary{
+				Platform: platformKey,
+				Status:   "Patched",
+				Ref:      res.PatchedRef.String(),
+				Error:    "",
+			}
 			log.Infof("Patched image (%s): %s\n", p.OS+"/"+p.Architecture, res.PatchedRef.String())
 			return nil
 		})
@@ -847,7 +867,23 @@ func patchMultiPlatformImage(
 		}
 	}
 
-	log.Infof("Multi-platform image patched with tag %s", patchedImageName.String())
+	var b strings.Builder
+	w := tabwriter.NewWriter(&b, 0, 0, 2, ' ', 0)
+	fmt.Fprintln(w, "PLATFORM\tSTATUS\tREFERENCE\tERROR")
+
+	for _, p := range platforms {
+		platformKey := buildkit.PlatformKey(p.Platform)
+		s := summaryMap[platformKey]
+		if s != nil {
+			ref := s.Ref
+			if ref == "" {
+				ref = "-"
+			}
+			fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", s.Platform, s.Status, ref, s.Error)
+		}
+	}
+	w.Flush()
+	log.Info("\nMulti-arch patch summary:\n" + b.String())
 
 	return nil
 }
diff --git a/pkg/types/types.go b/pkg/types/types.go
index 653496dc..40feb6ca 100644
--- a/pkg/types/types.go
+++ b/pkg/types/types.go
@@ -41,3 +41,10 @@ type PatchResult struct {
 	PatchedDesc *ispec.Descriptor
 	PatchedRef  reference.Named
 }
+
+type MultiArchSummary struct {
+	Platform string
+	Status   string
+	Ref      string
+	Error    string
+}


Test patch:
diff --git a/pkg/patch/patch_test.go b/pkg/patch/patch_test.go
index 03a65d55..b98a2bfd 100644
--- a/pkg/patch/patch_test.go
+++ b/pkg/patch/patch_test.go
@@ -7,6 +7,7 @@ import (
 	"os"
 	"strings"
 	"testing"
+	"text/tabwriter"
 	"time"
 
 	"github.com/distribution/reference"
@@ -16,6 +17,7 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	buildkitclient "github.com/moby/buildkit/client"
+	ispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 func TestRemoveIfNotDebug(t *testing.T) {
@@ -579,3 +581,69 @@ func TestNormalizeConfigForPlatform(t *testing.T) {
 		t.Errorf("variant key should be dropped when empty")
 	}
 }
+
+func TestMultiArchSummaryTable(t *testing.T) {
+	platforms := []struct {
+		OS           string
+		Architecture string
+		Variant      string
+	}{
+		{"linux", "amd64", ""},
+		{"linux", "arm64", ""},
+		{"linux", "arm", "v7"},
+	}
+
+	summaryMap := map[string]*types.MultiArchSummary{
+		"linux/amd64": {
+			Platform: "linux/amd64",
+			Status:   "Patched",
+			Ref:      "docker.io/library/nginx:patched-amd64",
+			Error:    "",
+		},
+		"linux/arm64": {
+			Platform: "linux/arm64",
+			Status:   "Error",
+			Ref:      "",
+			Error:    "emulation is not enabled for platform linux/arm64",
+		},
+		"linux/arm/v7": {
+			Platform: "linux/arm/v7",
+			Status:   "Ignored",
+			Ref:      "",
+			Error:    "",
+		},
+	}
+
+	var b strings.Builder
+	w := tabwriter.NewWriter(&b, 0, 0, 2, ' ', 0)
+	_, _ = w.Write([]byte("PLATFORM\tSTATUS\tREFERENCE\tERROR\n"))
+	for _, p := range platforms {
+		platformKey := buildkit.PlatformKey(ispec.Platform{
+			OS:           p.OS,
+			Architecture: p.Architecture,
+			Variant:      p.Variant,
+		})
+		s := summaryMap[platformKey]
+		if s != nil {
+			ref := s.Ref
+			if ref == "" {
+				ref = "-"
+			}
+			_, _ = w.Write([]byte(
+				s.Platform + "\t" + s.Status + "\t" + ref + "\t" + s.Error + "\n",
+			))
+		}
+	}
+	w.Flush()
+
+	got := b.String()
+	expected := `PLATFORM      STATUS   REFERENCE                              ERROR
+linux/amd64   Patched  docker.io/library/nginx:patched-amd64  
+linux/arm64   Error    -                                      emulation is not enabled for platform linux/arm64
+linux/arm/v7  Ignored  -                                      
+`
+
+	if got != expected {
+		t.Errorf("summary table output mismatch:\ngot:\n%s\nwant:\n%s", got, expected)
+	}
+}