From 2fe4450620a00a0bb7fe5542463be07052365fdc Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Mon, 11 May 2020 22:54:28 +0200 Subject: [PATCH] store/compose/ImageType: use distro.ImageType objects This reduces the amount of resolving and error checking we have to do. This exposed a bug in weldr's ComposeEntry type, which will be fixed in a follow-up commit. Signed-off-by: Tom Gundersen --- cmd/osbuild-composer/main.go | 2 +- internal/store/compose.go | 3 ++- internal/store/fixtures.go | 27 +++++++++++++++++++++------ internal/store/json.go | 33 +++++++++++++++++++++++++-------- internal/store/store.go | 20 ++++---------------- internal/store/store_test.go | 6 +++++- internal/weldr/api.go | 22 +++++----------------- internal/weldr/api_test.go | 10 +++++++--- internal/weldr/compose.go | 8 +++++++- 9 files changed, 77 insertions(+), 54 deletions(-) diff --git a/cmd/osbuild-composer/main.go b/cmd/osbuild-composer/main.go index 338049fc5..3d6f4293e 100644 --- a/cmd/osbuild-composer/main.go +++ b/cmd/osbuild-composer/main.go @@ -117,7 +117,7 @@ func main() { logger = log.New(os.Stdout, "", 0) } - store := store.New(&stateDir) + store := store.New(&stateDir, arch) queueDir := path.Join(stateDir, "jobs") err = os.Mkdir(queueDir, 0700) diff --git a/internal/store/compose.go b/internal/store/compose.go index d69e52bdf..4feb4cbae 100644 --- a/internal/store/compose.go +++ b/internal/store/compose.go @@ -6,6 +6,7 @@ import ( "github.com/google/uuid" "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/common" + "github.com/osbuild/osbuild-composer/internal/distro" "github.com/osbuild/osbuild-composer/internal/osbuild" "github.com/osbuild/osbuild-composer/internal/target" ) @@ -21,7 +22,7 @@ func (ste *StateTransitionError) Error() string { // ImageBuild represents a single image build inside a compose type ImageBuild struct { ID int - ImageType common.ImageType + ImageType distro.ImageType Manifest *osbuild.Manifest Targets []*target.Target JobCreated time.Time diff --git a/internal/store/fixtures.go b/internal/store/fixtures.go index bbcd819da..39b5848ef 100644 --- a/internal/store/fixtures.go +++ b/internal/store/fixtures.go @@ -6,6 +6,7 @@ import ( "github.com/google/uuid" "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/common" + test_distro "github.com/osbuild/osbuild-composer/internal/distro/fedoratest" "github.com/osbuild/osbuild-composer/internal/target" ) @@ -46,7 +47,16 @@ func FixtureBase() *Store { }, } - s := New(nil) + d := test_distro.New() + arch, err := d.GetArch("x86_64") + if err != nil { + panic("invalid architecture x86_64 for fedoratest") + } + imgType, err := arch.GetImageType("qcow2") + if err != nil { + panic("invalid image type qcow2 for x86_64 @ fedoratest") + } + s := New(nil, arch) s.blueprints[bName] = b s.composes = map[uuid.UUID]Compose{ @@ -55,7 +65,7 @@ func FixtureBase() *Store { ImageBuilds: []ImageBuild{ { QueueStatus: common.IBWaiting, - ImageType: common.Qcow2Generic, + ImageType: imgType, Targets: []*target.Target{localTarget, awsTarget}, JobCreated: date, }, @@ -66,7 +76,7 @@ func FixtureBase() *Store { ImageBuilds: []ImageBuild{ { QueueStatus: common.IBRunning, - ImageType: common.Qcow2Generic, + ImageType: imgType, Targets: []*target.Target{localTarget}, JobCreated: date, JobStarted: date, @@ -78,7 +88,7 @@ func FixtureBase() *Store { ImageBuilds: []ImageBuild{ { QueueStatus: common.IBFinished, - ImageType: common.Qcow2Generic, + ImageType: imgType, Targets: []*target.Target{localTarget, awsTarget}, JobCreated: date, JobStarted: date, @@ -91,7 +101,7 @@ func FixtureBase() *Store { ImageBuilds: []ImageBuild{ { QueueStatus: common.IBFailed, - ImageType: common.Qcow2Generic, + ImageType: imgType, Targets: []*target.Target{localTarget, awsTarget}, JobCreated: date, JobStarted: date, @@ -115,7 +125,12 @@ func FixtureEmpty() *Store { Customizations: nil, } - s := New(nil) + d := test_distro.New() + arch, err := d.GetArch("x86_64") + if err != nil { + panic("invalid architecture x86_64 for fedoratest") + } + s := New(nil, arch) s.blueprints[bName] = b diff --git a/internal/store/json.go b/internal/store/json.go index 0a580e615..9f8b80745 100644 --- a/internal/store/json.go +++ b/internal/store/json.go @@ -8,6 +8,7 @@ import ( "github.com/google/uuid" "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/common" + "github.com/osbuild/osbuild-composer/internal/distro" "github.com/osbuild/osbuild-composer/internal/osbuild" "github.com/osbuild/osbuild-composer/internal/target" ) @@ -90,20 +91,28 @@ func newWorkspaceFromV0(workspaceStruct workspaceV0) map[string]blueprint.Bluepr return workspace } -func newComposesFromV0(composesStruct composesV0) map[uuid.UUID]Compose { +func newComposesFromV0(composesStruct composesV0, arch distro.Arch) map[uuid.UUID]Compose { composes := make(map[uuid.UUID]Compose) for composeID, composeStruct := range composesStruct { c := Compose{ Blueprint: composeStruct.Blueprint, } - if len(composeStruct.ImageBuilds) == 0 { - panic("the was a compose with zero image builds, that is forbidden") - } for _, imgBuild := range composeStruct.ImageBuilds { + imgTypeString, valid := imgBuild.ImageType.ToCompatString() + if !valid { + // Invalid type strings in serialization format, this may happen + // on upgrades. Ignore the image build. + continue + } + imgType, err := arch.GetImageType(imgTypeString) + if err != nil { + // Ditto. + continue + } ib := ImageBuild{ ID: imgBuild.ID, - ImageType: imgBuild.ImageType, + ImageType: imgType, Manifest: imgBuild.Manifest, Targets: imgBuild.Targets, JobCreated: imgBuild.JobCreated, @@ -115,6 +124,10 @@ func newComposesFromV0(composesStruct composesV0) map[uuid.UUID]Compose { } c.ImageBuilds = append(c.ImageBuilds, ib) } + if len(c.ImageBuilds) == 0 { + // In case no valid image builds were found, ignore the compose. + continue + } composes[composeID] = c } @@ -158,11 +171,11 @@ func newCommitsFromV0(commitsStruct commitsV0) map[string][]string { return commits } -func newStoreFromV0(storeStruct storeV0) *Store { +func newStoreFromV0(storeStruct storeV0, arch distro.Arch) *Store { store := Store{ blueprints: newBlueprintsFromV0(storeStruct.Blueprints), workspace: newWorkspaceFromV0(storeStruct.Workspace), - composes: newComposesFromV0(storeStruct.Composes), + composes: newComposesFromV0(storeStruct.Composes, arch), sources: newSourceConfigsFromV0(storeStruct.Sources), blueprintsChanges: newChangesFromV0(storeStruct.Changes), blueprintsCommits: newCommitsFromV0(storeStruct.Commits), @@ -238,9 +251,13 @@ func newComposesV0(composes map[uuid.UUID]Compose) composesV0 { panic("the was a compose with zero image builds, that is forbidden") } for _, imgBuild := range compose.ImageBuilds { + imgType, valid := common.ImageTypeFromCompatString(imgBuild.ImageType.Name()) + if !valid { + panic("invalid image type") + } ib := imageBuildV0{ ID: imgBuild.ID, - ImageType: imgBuild.ImageType, + ImageType: imgType, Manifest: imgBuild.Manifest, Targets: imgBuild.Targets, JobCreated: imgBuild.JobCreated, diff --git a/internal/store/store.go b/internal/store/store.go index 99e9d5c53..58b86db8d 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -73,7 +73,7 @@ func (e *NoLocalTargetError) Error() string { return e.message } -func New(stateDir *string) *Store { +func New(stateDir *string, arch distro.Arch) *Store { var storeStruct storeV0 var db *jsondb.JSONDatabase @@ -90,7 +90,7 @@ func New(stateDir *string) *Store { } } - store := newStoreFromV0(storeStruct) + store := newStoreFromV0(storeStruct, arch) store.stateDir = stateDir store.db = db @@ -397,12 +397,6 @@ func (s *Store) PushCompose(composeID uuid.UUID, manifest *osbuild.Manifest, ima targets = []*target.Target{} } - // Compatibility layer for image types in Weldr API v0 - imageTypeCommon, exists := common.ImageTypeFromCompatString(imageType.Name()) - if !exists { - panic("fatal error, compose type does not exist") - } - if s.stateDir != nil { outputDir := s.getImageBuildDirectory(composeID, 0) @@ -419,7 +413,7 @@ func (s *Store) PushCompose(composeID uuid.UUID, manifest *osbuild.Manifest, ima ImageBuilds: []ImageBuild{ { Manifest: manifest, - ImageType: imageTypeCommon, + ImageType: imageType, Targets: targets, JobCreated: time.Now(), Size: size, @@ -440,12 +434,6 @@ func (s *Store) PushTestCompose(composeID uuid.UUID, manifest *osbuild.Manifest, targets = []*target.Target{} } - // Compatibility layer for image types in Weldr API v0 - imageTypeCommon, exists := common.ImageTypeFromCompatString(imageType.Name()) - if !exists { - panic("fatal error, compose type does not exist") - } - if s.stateDir != nil { outputDir := s.getImageBuildDirectory(composeID, 0) @@ -479,7 +467,7 @@ func (s *Store) PushTestCompose(composeID uuid.UUID, manifest *osbuild.Manifest, { QueueStatus: status, Manifest: manifest, - ImageType: imageTypeCommon, + ImageType: imageType, Targets: targets, JobCreated: time.Now(), JobStarted: time.Now(), diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 0949861c0..b87ebe6e5 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/osbuild/osbuild-composer/internal/blueprint" + "github.com/osbuild/osbuild-composer/internal/distro/test_distro" ) //struct for sharing state between tests @@ -50,8 +51,11 @@ func (suite *storeTest) SetupSuite() { func (suite *storeTest) SetupTest() { tmpDir, err := ioutil.TempDir("/tmp", "osbuild-composer-test-") suite.NoError(err) + distro := test_distro.New() + arch, err := distro.GetArch("test_arch") + suite.NoError(err) suite.dir = tmpDir - suite.myStore = New(&suite.dir) + suite.myStore = New(&suite.dir, arch) } //teardown after each test diff --git a/internal/weldr/api.go b/internal/weldr/api.go index 46153b69b..28ed60084 100644 --- a/internal/weldr/api.go +++ b/internal/weldr/api.go @@ -1800,7 +1800,7 @@ func (api *API) composeStatusHandler(writer http.ResponseWriter, request *http.R } filterStatus := q.Get("status") - filterImageType, filterImageTypeExists := common.ImageTypeFromCompatString(q.Get("type")) + filterImageType := q.Get("type") filteredUUIDs := []uuid.UUID{} for _, id := range uuids { @@ -1813,7 +1813,7 @@ func (api *API) composeStatusHandler(writer http.ResponseWriter, request *http.R continue } else if filterStatus != "" && state.ToString() != filterStatus { continue - } else if filterImageTypeExists && compose.ImageBuilds[0].ImageType != filterImageType { + } else if filterImageType != "" && compose.ImageBuilds[0].ImageType.Name() != filterImageType { continue } filteredUUIDs = append(filteredUUIDs, id) @@ -1884,7 +1884,7 @@ func (api *API) composeInfoHandler(writer http.ResponseWriter, request *http.Req // Weldr API assumes only one image build per compose, that's why only the // 1st build is considered state, _, _, _ := api.getComposeState(compose) - reply.ComposeType, _ = compose.ImageBuilds[0].ImageType.ToCompatString() + reply.ComposeType = compose.ImageBuilds[0].ImageType.Name() reply.QueueStatus = state.ToString() reply.ImageSize = compose.ImageBuilds[0].Size @@ -1932,20 +1932,8 @@ func (api *API) composeImageHandler(writer http.ResponseWriter, request *http.Re return } - imageBuild := compose.ImageBuilds[0] - imageType, _ := imageBuild.ImageType.ToCompatString() - imageTypeStruct, err := api.arch.GetImageType(imageType) - if err != nil { - errors := responseError{ - ID: "BadCompose", - Msg: fmt.Sprintf("Compose %s is ill-formed: output type %v is invalid for distro %s on %s", - uuidString, imageBuild.ImageType, api.distro.Name(), api.arch.Name()), - } - statusResponseError(writer, http.StatusInternalServerError, errors) - return - } - imageName := imageTypeStruct.Filename() - imageMime := imageTypeStruct.MIMEType() + imageName := compose.ImageBuilds[0].ImageType.Filename() + imageMime := compose.ImageBuilds[0].ImageType.MIMEType() reader, fileSize, err := api.store.GetImageBuildImage(uuid, 0) diff --git a/internal/weldr/api_test.go b/internal/weldr/api_test.go index f7f2d6280..781c11ab7 100644 --- a/internal/weldr/api_test.go +++ b/internal/weldr/api_test.go @@ -433,6 +433,10 @@ func TestBlueprintsDepsolve(t *testing.T) { } func TestCompose(t *testing.T) { + arch, err := test_distro.New().GetArch("x86_64") + require.NoError(t, err) + imgType, err := arch.GetImageType("qcow2") + require.NoError(t, err) expectedComposeLocal := &store.Compose{ Blueprint: &blueprint.Blueprint{ Name: "test", @@ -445,7 +449,7 @@ func TestCompose(t *testing.T) { ImageBuilds: []store.ImageBuild{ { QueueStatus: common.IBWaiting, - ImageType: common.Qcow2Generic, + ImageType: imgType, Targets: []*target.Target{ { // skip Uuid and Created fields - they are ignored @@ -470,7 +474,7 @@ func TestCompose(t *testing.T) { ImageBuilds: []store.ImageBuild{ { QueueStatus: common.IBWaiting, - ImageType: common.Qcow2Generic, + ImageType: imgType, Targets: []*target.Target{ { Name: "org.osbuild.aws", @@ -535,7 +539,7 @@ func TestCompose(t *testing.T) { // TODO: find some (reasonable) way to verify the contents of the pipeline composeStruct.ImageBuilds[0].Manifest = nil - if diff := cmp.Diff(composeStruct, *c.ExpectedCompose, test.IgnoreDates(), test.IgnoreUuids(), test.Ignore("Targets.Options.Location")); diff != "" { + if diff := cmp.Diff(composeStruct, *c.ExpectedCompose, test.IgnoreDates(), test.IgnoreUuids(), test.Ignore("Targets.Options.Location"), test.CompareImageTypes()); diff != "" { t.Errorf("%s: compose in store isn't the same as expected, diff:\n%s", c.Path, diff) } diff --git a/internal/weldr/compose.go b/internal/weldr/compose.go index 678c18b5b..d5b8a6606 100644 --- a/internal/weldr/compose.go +++ b/internal/weldr/compose.go @@ -28,7 +28,13 @@ func composeToComposeEntry(id uuid.UUID, compose store.Compose, state common.Com composeEntry.ID = id composeEntry.Blueprint = compose.Blueprint.Name composeEntry.Version = compose.Blueprint.Version - composeEntry.ComposeType = compose.ImageBuilds[0].ImageType + + var valid bool + // BUG: this leads to the wrong image type string being returned, we want the compat strings here + composeEntry.ComposeType, valid = common.ImageTypeFromCompatString(compose.ImageBuilds[0].ImageType.Name()) + if !valid { + panic("compose contains invalid image type: " + compose.ImageBuilds[0].ImageType.Name()) + } if includeUploads { composeEntry.Uploads = targetsToUploadResponses(compose.ImageBuilds[0].Targets)