refactor: enhance artifact handling and validation logic

Author: asdek

models/artefacts.go

@@ -5,8 +5,8 @@ import (
 	"strings"
 )
 
-// ArtifactKind is how a component reaches an installation: pulled from a
-// registry, or downloaded as a file.
+// ArtifactKind is what artefacts exist for a component: a pulled image, a
+// downloaded file, or both.
 //
 // It is a property of the COMPONENT — fixed by how it is built and shipped —
 // and not of the request that mentions it. Deriving it from which fields a
@@ -24,6 +24,18 @@ const (
 	// Files have no repository and no tag, so the whole tag-and-channel half of
 	// update resolution is inapplicable to them.
 	ArtifactKindFile ArtifactKind = "file"
+
+	// ArtifactKindFileImage is both: pulled from a registry like an image, and
+	// published as a downloadable executable like a file.
+	//
+	// pentagi is one. An installer pulls the container and reports it under
+	// `images`; a running product reports the executable under `files` and can
+	// replace it without waiting for a pull. The two facts are one kind, not a
+	// kind plus a side table: moving pentagi to ArtifactKindFile would take it
+	// out of every update check's `images` list, and leaving it as
+	// ArtifactKindImage would refuse it under `files` and refuse the download
+	// of its own executable.
+	ArtifactKindFileImage ArtifactKind = "file-image"
 )
 
 func (ak ArtifactKind) String() string {
@@ -32,13 +44,33 @@ func (ak ArtifactKind) String() string {
 
 func (ak ArtifactKind) Valid() error {
 	switch ak {
-	case ArtifactKindImage, ArtifactKindFile:
+	case ArtifactKindImage, ArtifactKindFile, ArtifactKindFileImage:
 		return nil
 	default:
 		return fmt.Errorf("invalid ArtifactKind: %s", ak)
 	}
 }
 
+// HasFile reports whether a downloadable file is published for this kind.
+func (ak ArtifactKind) HasFile() bool {
+	switch ak {
+	case ArtifactKindFile, ArtifactKindFileImage:
+		return true
+	default:
+		return false
+	}
+}
+
+// HasImage reports whether this kind is pulled from a registry.
+func (ak ArtifactKind) HasImage() bool {
+	switch ak {
+	case ArtifactKindImage, ArtifactKindFileImage:
+		return true
+	default:
+		return false
+	}
+}
+
 // componentArtifactKinds is THE table. Every component of AllComponentTypes has
 // an entry here, and TestEveryComponentHasAnArtifactKind fails when one does
 // not.
@@ -59,9 +91,12 @@ var componentArtifactKinds = map[ComponentType]ArtifactKind{
 	ComponentTypeEngineScenario:   ArtifactKindFile,
 	ComponentTypeJaegerClickhouse: ArtifactKindFile,
 
+	// Both: the installer pulls a container, and a running product can also
+	// download the executable of the same build.
+	ComponentTypePentagi: ArtifactKindFileImage,
+
 	// Delivered as images: everything published to a registry, plus every
 	// third-party service a stack starts from one.
-	ComponentTypePentagi:         ArtifactKindImage,
 	ComponentTypeScraper:         ArtifactKindImage,
 	ComponentTypeWorker:          ArtifactKindImage,
 	ComponentTypePgvector:        ArtifactKindImage,
@@ -97,28 +132,34 @@ func (ct ComponentType) ArtifactKind() ArtifactKind {
 	return ArtifactKindImage
 }
 
-// IsFileComponent reports whether this component is delivered as a file.
+// IsFileComponent reports whether this component can appear under `files` and
+// whether a package of it can be downloaded. FileImage components are included:
+// they have that file in addition to the image an installer pulls.
 func (ct ComponentType) IsFileComponent() bool {
-	return ct.ArtifactKind() == ArtifactKindFile
+	return ct.ArtifactKind().HasFile()
 }
 
-// IsImageComponent reports whether this component is delivered as an image.
+// IsImageComponent reports whether this component can appear under `images`.
+// FileImage components are included: that is how an installer delivers them.
 func (ct ComponentType) IsImageComponent() bool {
-	return ct.ArtifactKind() == ArtifactKindImage
+	return ct.ArtifactKind().HasImage()
 }
 
-// FileComponentTypes and ImageComponentTypes are the vocabulary split by kind,
-// in vocabulary order. Derived from the table rather than written twice, so
-// there is no second list to forget.
+// FileComponentTypes and ImageComponentTypes are the vocabulary split by which
+// artefacts exist, in vocabulary order. Derived from the table rather than
+// written twice, so there is no second list to forget.
+//
+// They overlap: a FileImage component is in both, which is the whole point of
+// that kind.
 var (
-	FileComponentTypes  = componentsOfKind(ArtifactKindFile)
-	ImageComponentTypes = componentsOfKind(ArtifactKindImage)
+	FileComponentTypes  = componentsMatching(ArtifactKind.HasFile)
+	ImageComponentTypes = componentsMatching(ArtifactKind.HasImage)
 )
 
-func componentsOfKind(kind ArtifactKind) []ComponentType {
+func componentsMatching(match func(ArtifactKind) bool) []ComponentType {
 	out := make([]ComponentType, 0, len(AllComponentTypes))
 	for _, ct := range AllComponentTypes {
-		if ct.ArtifactKind() == kind {
+		if match(ct.ArtifactKind()) {
 			out = append(out, ct)
 		}
 	}
@@ -128,8 +169,12 @@ func componentsOfKind(kind ArtifactKind) []ComponentType {
 // FileComponentEnum is the file vocabulary as a comma-separated list, the form
 // documentation and hand-written enumerations have to match.
 func FileComponentEnum() string {
-	names := make([]string, 0, len(FileComponentTypes))
-	for _, ct := range FileComponentTypes {
+	return componentEnum(FileComponentTypes)
+}
+
+func componentEnum(components []ComponentType) string {
+	names := make([]string, 0, len(components))
+	for _, ct := range components {
 		names = append(names, ct.String())
 	}
 	return strings.Join(names, ",")

models/artefacts_test.go

@@ -33,15 +33,17 @@ func TestEveryComponentHasAnArtifactKind(t *testing.T) {
 	}
 }
 
-// TestTheFileVocabularyIsExactlyTheDeliveredFiles pins the split itself, as a
-// literal, on both sides of the contract.
+// TestTheFileVocabularyIsExactlyTheDeliveredFiles pins which components have a
+// published file — the file-only ones plus FileImage — as a literal on both
+// sides of the contract.
 func TestTheFileVocabularyIsExactlyTheDeliveredFiles(t *testing.T) {
 	want := []ComponentType{
 		ComponentTypeInstaller,
 		ComponentTypeEngine,
 		ComponentTypeJaegerClickhouse,
 		ComponentTypeEngineRegistry,
 		ComponentTypeEngineScenario,
+		ComponentTypePentagi,
 	}
 	got := map[ComponentType]bool{}
 	for _, ct := range FileComponentTypes {
@@ -56,22 +58,34 @@ func TestTheFileVocabularyIsExactlyTheDeliveredFiles(t *testing.T) {
 	for ct := range got {
 		t.Errorf("%s is delivered as a file but is not in the pinned list", ct)
 	}
-	if n := len(FileComponentTypes) + len(ImageComponentTypes); n != len(AllComponentTypes) {
-		t.Errorf("the two kinds cover %d components, the vocabulary has %d", n, len(AllComponentTypes))
+	// FileImage sits in both lists, so the two lengths sum to more than the
+	// vocabulary. Every component must still appear in at least one.
+	seen := map[ComponentType]struct{}{}
+	for _, ct := range FileComponentTypes {
+		seen[ct] = struct{}{}
+	}
+	for _, ct := range ImageComponentTypes {
+		seen[ct] = struct{}{}
+	}
+	if len(seen) != len(AllComponentTypes) {
+		t.Errorf("the two kinds cover %d components, the vocabulary has %d",
+			len(seen), len(AllComponentTypes))
 	}
 }
 
 // TestOnlyAFileHasAPackage exercises the `filecomp` tag over the WHOLE
 // vocabulary — the shape that catches an omission rather than one that confirms
 // the members already present.
 //
-// The list this replaced named two of the five, so an SDK asking for
-// jaeger-clickhouse, engine-registry or engine-scenario refused locally before
-// the request was ever sent.
+// The list this replaced named two of the five file-only components, so an SDK
+// asking for jaeger-clickhouse, engine-registry or engine-scenario refused
+// locally before the request was ever sent. Asking only the image/file delivery
+// question then cost one more: pentagi is FileImage, and treating that as "not
+// a file" refused the executable a running product replaces itself with.
 //
-// BOTH package requests are swept. Gating one and not the other is a real
-// state this test exists to forbid: the SDK would send a metadata request the
-// service refuses, while refusing the download of that same artefact itself.
+// BOTH package requests are swept. Gating one and not the other is a real state
+// this test exists to forbid: the SDK would send a metadata request the service
+// refuses, while refusing the download of that same artefact itself.
 func TestOnlyAFileHasAPackage(t *testing.T) {
 	for _, ct := range AllComponentTypes {
 		requests := map[string]IValid{
@@ -90,20 +104,71 @@ func TestOnlyAFileHasAPackage(t *testing.T) {
 		}
 		for name, request := range requests {
 			err := request.Valid()
-			switch ct.ArtifactKind() {
-			case ArtifactKindFile:
-				if err != nil {
-					t.Errorf("%s is delivered as a file but %s rejects it: %v", ct, name, err)
-				}
-			case ArtifactKindImage:
-				if err == nil {
-					t.Errorf("%s is an image and has no package, but %s accepted it", ct, name)
-				}
+			if ct.IsFileComponent() && err != nil {
+				t.Errorf("%s has a published file but %s rejects it: %v", ct, name, err)
+			}
+			if !ct.IsFileComponent() && err == nil {
+				t.Errorf("%s has no package, but %s accepted it", ct, name)
 			}
 		}
 	}
 }
 
+// TestPentagiIsFileImage pins the one component that is both an image an
+// installer pulls and a file a running product downloads.
+//
+// It must stay in BOTH update-check lists: under `images` the service resolves
+// it against its registry reference, under `files` against the published
+// executable. Moving it to ArtifactKindFile takes it out of every installer's
+// image list; leaving it as ArtifactKindImage refuses the file half. FileImage
+// is both answers in the table, not a kind plus a side set.
+func TestPentagiIsFileImage(t *testing.T) {
+	if got := ComponentTypePentagi.ArtifactKind(); got != ArtifactKindFileImage {
+		t.Errorf("pentagi must be file-image, got %s", got)
+	}
+	if !ComponentTypePentagi.IsImageComponent() {
+		t.Error("pentagi must stay an image: an update check resolves it as one")
+	}
+	if !ComponentTypePentagi.IsFileComponent() {
+		t.Error("pentagi ships an executable a running product can replace itself with")
+	}
+
+	base := CheckUpdatesRequest{
+		InstallerVersion: "1.0.0",
+		InstallerOS:      OSTypeLinux,
+		InstallerArch:    ArchTypeAMD64,
+		Strategy:         UpdateStrategyPreview,
+	}
+	asFile := base
+	asFile.Files = []FileComponentInfo{{
+		Component: ComponentTypePentagi,
+		Status:    ComponentStatusRunning,
+		OS:        OSTypeLinux,
+		Arch:      ArchTypeAMD64,
+	}}
+	if err := asFile.Valid(); err != nil {
+		t.Errorf("pentagi reported under `files` must validate: %v", err)
+	}
+	asImage := base
+	asImage.Images = []ImageComponentInfo{{
+		Component:  ComponentTypePentagi,
+		Status:     ComponentStatusRunning,
+		OS:         OSTypeLinux,
+		Arch:       ArchTypeAMD64,
+		Repository: "vxcontrol/pentagi",
+		Tag:        "latest",
+	}}
+	if err := asImage.Valid(); err != nil {
+		t.Errorf("pentagi reported under `images` must validate: %v", err)
+	}
+	both := base
+	both.Images = asImage.Images
+	both.Files = asFile.Files
+	if err := both.Valid(); err != nil {
+		t.Errorf("pentagi reported under both lists must validate: %v", err)
+	}
+}
+
 // TestAComponentReportedUnderTheWrongKindIsRejected closes the hole that
 // splitting the request lists left open.
 //
@@ -126,17 +191,17 @@ func TestAComponentReportedUnderTheWrongKindIsRejected(t *testing.T) {
 	t.Run("an image reported under files", func(t *testing.T) {
 		request := base()
 		request.Files = []FileComponentInfo{{
-			Component: ComponentTypePentagi,
+			Component: ComponentTypeScraper,
 			Status:    ComponentStatusRunning,
 			OS:        OSTypeLinux,
 			Arch:      ArchTypeAMD64,
 			Version:   &version,
 		}}
 		err := request.Valid()
 		if err == nil {
-			t.Fatal("pentagi is an image; reporting it as a file must not validate")
+			t.Fatal("scraper is an image; reporting it as a file must not validate")
 		}
-		if !strings.Contains(err.Error(), "pentagi") {
+		if !strings.Contains(err.Error(), "scraper") {
 			t.Errorf("the error does not name the component: %v", err)
 		}
 	})