Repository: osbuild/osbuild-composer
Pull request #382 (2020-03-23 19:10:46)

Store cleanups
Some random cleanups of the store and its interaction with weldr, while I'm continuing to research towards #342.

Hope the commits are self-explanatory.

larskarlitski: > Love these cleanups! In addition to request to one of the patches, a `go fmt` is needed.

Thanks for the review! Should be good for another round.
larskarlitski: Update to resolve the conflict.
larskarlitski: This [passed on travis](https://travis-ci.org/github/larskarlitski/osbuild-composer/builds/666522179), but it failed to send status.

Patch:
diff --git a/internal/jobqueue/api.go b/internal/jobqueue/api.go
index f68200a2b..bd017c3e4 100644
--- a/internal/jobqueue/api.go
+++ b/internal/jobqueue/api.go
@@ -107,7 +107,6 @@ func (api *API) addJobHandler(writer http.ResponseWriter, request *http.Request,
 		ImageBuildID: nextJob.ImageBuildID,
 		Manifest:     nextJob.Manifest,
 		Targets:      nextJob.Targets,
-		OutputType:   nextJob.ImageType,
 	})
 }
 
diff --git a/internal/jobqueue/job.go b/internal/jobqueue/job.go
index 49c31e7c5..9de7592ef 100644
--- a/internal/jobqueue/job.go
+++ b/internal/jobqueue/job.go
@@ -22,7 +22,6 @@ type Job struct {
 	ImageBuildID int               `json:"image_build_id"`
 	Manifest     *osbuild.Manifest `json:"manifest"`
 	Targets      []*target.Target  `json:"targets"`
-	OutputType   string            `json:"output_type"`
 }
 
 type JobStatus struct {
diff --git a/internal/store/store.go b/internal/store/store.go
index 22a713659..950b330f3 100644
--- a/internal/store/store.go
+++ b/internal/store/store.go
@@ -54,7 +54,6 @@ type Job struct {
 	ImageBuildID int
 	Manifest     *osbuild.Manifest
 	Targets      []*target.Target
-	ImageType    string
 }
 
 type SourceConfig struct {
@@ -163,16 +162,11 @@ func New(stateDir *string) *Store {
 					// s.Composes[composeID] = compose
 				case common.IBWaiting:
 					// Push waiting composes back into the pending jobs queue
-					imageTypeCompat, exists := imgBuild.ImageType.ToCompatString()
-					if !exists {
-						panic("fatal error, image type tag should exist but does not")
-					}
 					s.pendingJobs <- Job{
 						ComposeID:    composeID,
 						ImageBuildID: imgBuild.Id,
 						Manifest:     imgBuild.Manifest,
 						Targets:      imgBuild.Targets,
-						ImageType:    imageTypeCompat,
 					}
 				}
 			}
@@ -560,8 +554,10 @@ func (s *Store) getImageBuildDirectory(composeID uuid.UUID, imageBuildID int) st
 	return fmt.Sprintf("%s/%d", s.getComposeDirectory(composeID), imageBuildID)
 }
 
-func (s *Store) PushCompose(distro distro.Distro, arch distro.Arch, imageType distro.ImageType, composeID uuid.UUID, bp *blueprint.Blueprint, repos []rpmmd.RepoConfig, packages, buildPackages []rpmmd.PackageSpec, size uint64, uploadTarget *target.Target) error {
-	targets := []*target.Target{}
+func (s *Store) PushCompose(distro distro.Distro, arch distro.Arch, imageType distro.ImageType, composeID uuid.UUID, bp *blueprint.Blueprint, repos []rpmmd.RepoConfig, packages, buildPackages []rpmmd.PackageSpec, size uint64, targets []*target.Target) error {
+	if targets == nil {
+		targets = []*target.Target{}
+	}
 
 	// Compatibility layer for image types in Weldr API v0
 	imageTypeCommon, exists := common.ImageTypeFromCompatString(imageType.Name())
@@ -576,16 +572,6 @@ func (s *Store) PushCompose(distro distro.Distro, arch distro.Arch, imageType di
 		if err != nil {
 			return fmt.Errorf("cannot create output directory for job %v: %#v", composeID, err)
 		}
-
-		targets = append(targets, target.NewLocalTarget(
-			&target.LocalTargetOptions{
-				Filename: imageType.Filename(),
-			},
-		))
-	}
-
-	if uploadTarget != nil {
-		targets = append(targets, uploadTarget)
 	}
 
 	allRepos := append([]rpmmd.RepoConfig{}, repos...)
@@ -619,7 +605,6 @@ func (s *Store) PushCompose(distro distro.Distro, arch distro.Arch, imageType di
 		ImageBuildID: 0,
 		Manifest:     manifestStruct,
 		Targets:      targets,
-		ImageType:    imageType.Name(),
 	}
 
 	return nil
@@ -676,23 +661,10 @@ func (s *Store) PopJob() Job {
 		if !exists {
 			panic("Invalid job in queue.")
 		}
-		// Loop over the image builds and find the right one according to the image type
 		// Change queue status to running for the image build as well as for the targets
-		for n, imageBuild := range compose.ImageBuilds {
-			imgTypeCompatStr, exists := imageBuild.ImageType.ToCompatString()
-			if !exists {
-				panic("fatal error, image type should exist but it does not")
-			}
-			if imgTypeCompatStr != job.ImageType {
-				continue
-			}
-			if imageBuild.QueueStatus != common.IBWaiting {
-				panic("Invalid job in queue.")
-			}
-			compose.ImageBuilds[n].QueueStatus = common.IBRunning
-			for m := range compose.ImageBuilds[n].Targets {
-				compose.ImageBuilds[n].Targets[m].Status = common.IBRunning
-			}
+		compose.ImageBuilds[job.ImageBuildID].QueueStatus = common.IBRunning
+		for m := range compose.ImageBuilds[job.ImageBuildID].Targets {
+			compose.ImageBuilds[job.ImageBuildID].Targets[m].Status = common.IBRunning
 		}
 		// Replace the compose struct with the new one
 		// TODO: I'm not sure this is needed, but I don't know what is the golang semantics in this case
diff --git a/internal/weldr/api.go b/internal/weldr/api.go
index 76627fc76..00dba86ca 100644
--- a/internal/weldr/api.go
+++ b/internal/weldr/api.go
@@ -1407,11 +1407,18 @@ func (api *API) composeHandler(writer http.ResponseWriter, request *http.Request
 		return
 	}
 
-	var uploadTarget *target.Target
+	var targets []*target.Target
 	if isRequestVersionAtLeast(params, 1) && cr.Upload != nil {
-		uploadTarget = uploadRequestToTarget(*cr.Upload, imageType)
+		t := uploadRequestToTarget(*cr.Upload, imageType)
+		targets = append(targets, t)
 	}
 
+	targets = append(targets, target.NewLocalTarget(
+		&target.LocalTargetOptions{
+			Filename: imageType.Filename(),
+		},
+	))
+
 	bp := api.store.GetBlueprintCommitted(cr.BlueprintName)
 	if bp == nil {
 		errors := responseError{
@@ -1437,7 +1444,7 @@ func (api *API) composeHandler(writer http.ResponseWriter, request *http.Request
 		Status:  true,
 	}
 
-	err = api.store.PushCompose(api.distro, api.arch, imageType, reply.BuildID, bp, api.repos, packages, buildPackages, cr.Size, uploadTarget)
+	err = api.store.PushCompose(api.distro, api.arch, imageType, reply.BuildID, bp, api.allRepositories(), packages, buildPackages, cr.Size, targets)
 
 	// TODO: we should probably do some kind of blueprint validation in future
 	// for now, let's just 500 and bail out
@@ -1958,12 +1965,7 @@ func (api *API) composeFailedHandler(writer http.ResponseWriter, request *http.R
 }
 
 func (api *API) fetchPackageList() (rpmmd.PackageList, error) {
-	repos := append([]rpmmd.RepoConfig{}, api.repos...)
-	for _, source := range api.store.GetAllSources() {
-		repos = append(repos, source.RepoConfig())
-	}
-
-	packages, _, err := api.rpmmd.FetchMetadata(repos, api.distro.ModulePlatformID(), api.arch.Name())
+	packages, _, err := api.rpmmd.FetchMetadata(api.allRepositories(), api.distro.ModulePlatformID(), api.arch.Name())
 	return packages, err
 }
 
@@ -1978,11 +1980,17 @@ func getPkgNameGlob(pkg blueprint.Package) string {
 	return pkg.Name
 }
 
-func (api *API) depsolveBlueprint(bp *blueprint.Blueprint, imageType distro.ImageType) ([]rpmmd.PackageSpec, []rpmmd.PackageSpec, error) {
+// Returns all configured repositories (base + sources) as rpmmd.RepoConfig
+func (api *API) allRepositories() []rpmmd.RepoConfig {
 	repos := append([]rpmmd.RepoConfig{}, api.repos...)
 	for _, source := range api.store.GetAllSources() {
 		repos = append(repos, source.RepoConfig())
 	}
+	return repos
+}
+
+func (api *API) depsolveBlueprint(bp *blueprint.Blueprint, imageType distro.ImageType) ([]rpmmd.PackageSpec, []rpmmd.PackageSpec, error) {
+	repos := api.allRepositories()
 	var specs []string = []string{}
 	for _, pkg := range bp.Packages {
 		specs = append(specs, getPkgNameGlob(pkg))


Test patch:
diff --git a/internal/jobqueue/api_test.go b/internal/jobqueue/api_test.go
index ab88fc6c6..526e1f256 100644
--- a/internal/jobqueue/api_test.go
+++ b/internal/jobqueue/api_test.go
@@ -58,7 +58,7 @@ func TestCreate(t *testing.T) {
 	}
 
 	test.TestRoute(t, api, false, "POST", "/job-queue/v1/jobs", `{}`, http.StatusCreated,
-		`{"id":"ffffffff-ffff-ffff-ffff-ffffffffffff","image_build_id":0,"manifest":{"sources":{},"pipeline":{}},"targets":[],"output_type":"qcow2"}`, "created", "uuid")
+		`{"id":"ffffffff-ffff-ffff-ffff-ffffffffffff","image_build_id":0,"manifest":{"sources":{},"pipeline":{}},"targets":[]}`, "created", "uuid")
 }
 
 func testUpdateTransition(t *testing.T, from, to string, expectedStatus int, expectedResponse string) {
diff --git a/internal/weldr/api_test.go b/internal/weldr/api_test.go
index 682c076ec..27a9d35a6 100644
--- a/internal/weldr/api_test.go
+++ b/internal/weldr/api_test.go
@@ -476,7 +476,15 @@ func TestCompose(t *testing.T) {
 			{
 				QueueStatus: common.IBWaiting,
 				ImageType:   common.Qcow2Generic,
-				Targets:     []*target.Target{},
+				Targets: []*target.Target{
+					{
+						// skip Uuid and Created fields - they are ignored
+						Name: "org.osbuild.local",
+						Options: &target.LocalTargetOptions{
+							Filename: "test.img",
+						},
+					},
+				},
 			},
 		},
 	}
@@ -507,6 +515,13 @@ func TestCompose(t *testing.T) {
 							Key:             "imagekey",
 						},
 					},
+					{
+						// skip Uuid and Created fields - they are ignored
+						Name: "org.osbuild.local",
+						Options: &target.LocalTargetOptions{
+							Filename: "test.img",
+						},
+					},
 				},
 			},
 		},