refactor: Trim Comments To Non-Obvious Rationale

Signed-off-by: Vad1mo <vadim@8gears.com>

Author: Vad1mo

src/controller/artifact/abstractor_test.go

@@ -387,12 +387,11 @@ var (
 
 type abstractorTestSuite struct {
 	suite.Suite
-	argMgr     *tart.Manager
-	blobMgr    *tblob.Manager
-	regCli     *registry.Client
-	abstractor *abstractor
-	processor  *tpro.Processor
-	// the classifier chain in place before the suite replaced it
+	argMgr      *tart.Manager
+	blobMgr     *tblob.Manager
+	regCli      *registry.Client
+	abstractor  *abstractor
+	processor   *tpro.Processor
 	classifiers []manifest.ChildClassifier
 }
 
@@ -409,9 +408,7 @@ func (a *abstractorTestSuite) SetupTest() {
 	// clear all registered processors
 	processor.Registry = map[string]processor.Processor{}
 	processor.Registry[schema2.MediaTypeImageConfig] = a.processor
-	// replace the classifier chain with a mock-backed instance so index children
-	// are classified against the suite's managers instead of the globals wired
-	// up in the manifest package's init()
+	// classify against the suite's managers, not the globals wired up by init()
 	a.classifiers = manifest.ChildClassifiers
 	manifest.ChildClassifiers = []manifest.ChildClassifier{
 		manifest.NewInTotoAttestationClassifier(a.argMgr, a.regCli),

src/controller/artifact/manifest/attestation.go

@@ -32,9 +32,8 @@ import (
 	"github.com/goharbor/harbor/src/pkg/registry"
 )
 
-// Attestation manifests are stored inside the index next to the platform images
-// they describe, linked by these annotations rather than by a distinct manifest
-// media type. See
+// Attestations are linked to the images they describe by these annotations
+// rather than by a distinct manifest media type.
 // https://github.com/moby/buildkit/blob/master/docs/attestations/attestation-storage.md
 const (
 	referenceTypeAnnotation         = "vnd.docker.reference.type"
@@ -57,25 +56,22 @@ type inTotoSubject struct {
 	Digest map[string]string `json:"digest"`
 }
 
-// InTotoAttestationClassifier classifies in-toto attestation manifests carried
-// inside an index as accessories of the platform image they attest. BuildKit,
-// Buildah and Podman all emit this layout, so the classifier is named after the
-// attestation format rather than after any single producer.
+// InTotoAttestationClassifier classifies in-toto attestation manifests as
+// accessories of the image they attest. Named after the attestation format, not
+// its producer: BuildKit, Buildah and Podman all emit this layout.
 type InTotoAttestationClassifier struct {
 	artMgr artifact.Manager
 	regCli registry.Client
 }
 
-// NewInTotoAttestationClassifier returns a classifier for in-toto attestation
-// manifests.
+// NewInTotoAttestationClassifier returns a classifier for in-toto attestations.
 func NewInTotoAttestationClassifier(artMgr artifact.Manager, regCli registry.Client) *InTotoAttestationClassifier {
 	return &InTotoAttestationClassifier{
 		artMgr: artMgr,
 		regCli: regCli,
 	}
 }
 
-// Classify implements ChildClassifier.
 func (c *InTotoAttestationClassifier) Classify(ctx context.Context, repository string, descriptor v1.Descriptor, siblings []v1.Descriptor) (*artifact.AccessoryCandidate, error) {
 	if !isAttestationDescriptor(descriptor) {
 		//nolint:nilnil // descriptor is not an attestation
@@ -223,11 +219,9 @@ func digestInIndex(siblings []v1.Descriptor, digestRef string) bool {
 	return false
 }
 
-// uniqueDigestInIndex returns the one sibling digest the subject matches. A
-// subject names a single artifact under several algorithms, so matching more
-// than one sibling means the payload disagrees with the index and is not safe
-// to attach. Rejecting the ambiguity also keeps the result independent of the
-// map iteration order behind subjectDigests.
+// A subject names one artifact under several algorithms, so matching more than
+// one sibling means the payload disagrees with the index. Rejecting that also
+// keeps the result independent of subjectDigests' map iteration order.
 func uniqueDigestInIndex(siblings []v1.Descriptor, digestRefs []string) string {
 	match := ""
 	for _, digestRef := range digestRefs {

src/controller/artifact/manifest/attestation_test.go

@@ -368,8 +368,7 @@ func TestResolveAttestationSubject(t *testing.T) {
 	})
 }
 
-// A subject names one artifact under several algorithms, so the digest map is
-// iterated in unspecified order. Resolution must not depend on that order.
+// Resolution must not depend on the digest map's iteration order.
 func TestResolveAttestationSubjectAmbiguousDigests(t *testing.T) {
 	amd64Encoded := "cad250bb95ea402adf4f687cc7d6747ecf0de875e6d6117f74437893964903df"
 	sha256Ref := "sha256:" + amd64Encoded

src/controller/artifact/manifest/classifier.go

@@ -23,24 +23,18 @@ import (
 	"github.com/goharbor/harbor/src/pkg/artifact"
 )
 
-// ChildClassifier inspects a child descriptor of an OCI index or a Docker
-// manifest list and decides whether it is a platform child of the index or an
-// accessory of one of its siblings.
+// ChildClassifier classifies a child descriptor of an OCI index or Docker manifest list.
 type ChildClassifier interface {
-	// Classify returns a non-nil candidate when the descriptor is an accessory of
-	// a sibling. Returning a nil candidate and a nil error means "not mine" and
-	// lets the next classifier decide.
+	// Classify returns a nil candidate and a nil error when the descriptor is not
+	// its concern, leaving it to the next classifier.
 	Classify(ctx context.Context, repository string, descriptor v1.Descriptor, siblings []v1.Descriptor) (*artifact.AccessoryCandidate, error)
 }
 
-// ChildClassifiers holds the registered classifiers in registration order.
-// Exported so that tests can replace the chain with mock-backed instances, the
-// same way tests replace processor.Registry.
+// ChildClassifiers holds the registered classifiers, in registration order.
 var ChildClassifiers []ChildClassifier
 
-// RegisterChildClassifier appends a classifier to the chain. A nil classifier is
-// dropped rather than stored: it would otherwise panic on every index push,
-// long after the faulty registration.
+// RegisterChildClassifier appends a classifier to the chain. A nil would panic on
+// every index push, far from the faulty registration, so it is dropped here.
 func RegisterChildClassifier(classifier ChildClassifier) {
 	if classifier == nil {
 		log.Errorf("refusing to register a nil child classifier")
@@ -49,9 +43,8 @@ func RegisterChildClassifier(classifier ChildClassifier) {
 	ChildClassifiers = append(ChildClassifiers, classifier)
 }
 
-// ClassifyChild walks the registered classifiers and returns the first accessory
-// candidate produced for the descriptor. A nil candidate means no classifier
-// claimed it, so the descriptor is an ordinary platform child of the index.
+// ClassifyChild returns the first accessory candidate claimed for the descriptor.
+// A nil candidate means it is an ordinary platform child of the index.
 func ClassifyChild(ctx context.Context, repository string, descriptor v1.Descriptor, siblings []v1.Descriptor) (*artifact.AccessoryCandidate, error) {
 	for _, classifier := range ChildClassifiers {
 		candidate, err := classifier.Classify(ctx, repository, descriptor, siblings)

src/controller/artifact/manifest/classifier_test.go

@@ -39,17 +39,14 @@ func (s stubClassifier) Classify(_ context.Context, _ string, _ v1.Descriptor, _
 	return s.candidate, s.err
 }
 
-// withClassifiers swaps the package chain for the duration of a test.
 func withClassifiers(t *testing.T, classifiers ...ChildClassifier) {
 	t.Helper()
 	original := ChildClassifiers
 	ChildClassifiers = classifiers
 	t.Cleanup(func() { ChildClassifiers = original })
 }
 
-// TestDefaultChainIsRegistered pins the bootstrap. An empty chain means index
-// children are never classified as accessories, which is silent data loss
-// rather than a visible failure.
+// An empty chain is silent data loss, not a visible failure, so pin the bootstrap.
 func TestDefaultChainIsRegistered(t *testing.T) {
 	require.NotEmpty(t, ChildClassifiers, "the package init() must register at least one classifier")
 
@@ -62,9 +59,7 @@ func TestDefaultChainIsRegistered(t *testing.T) {
 	}
 	require.NotNil(t, attestation, "the in-toto attestation classifier must be registered by default")
 
-	// init() captures pkg.ArtifactMgr and registry.Cli. Go initialises imported
-	// packages first, so both must already be set; a nil here would mean silent
-	// nil-pointer panics on every index push.
+	// init() captures these globals, so it relies on their packages initialising first
 	assert.NotNil(t, attestation.artMgr, "pkg.ArtifactMgr must be initialised before this package's init()")
 	assert.NotNil(t, attestation.regCli, "registry.Cli must be initialised before this package's init()")
 }
@@ -130,7 +125,6 @@ func TestRegisterChildClassifierDropsNil(t *testing.T) {
 	RegisterChildClassifier(nil)
 	assert.Empty(t, ChildClassifiers, "a nil classifier would panic on every index push")
 
-	// the chain stays usable
 	candidate, err := ClassifyChild(context.Background(), "library/hello", v1.Descriptor{}, nil)
 	require.NoError(t, err)
 	assert.Nil(t, candidate)