From 93fd5e3821a933e93a68550b868fa8af8503c154 Mon Sep 17 00:00:00 2001 From: Tomas Hozza Date: Mon, 13 Jun 2022 16:11:46 +0200 Subject: [PATCH] target: ensure that each used target has `NewXXXTargetResult` defined Ensure that `UnmarshalTargetResultOptions()` is called only when there are any options to unmarshal in the JSON object. Since results of some Targets don't have any options defined, mark `TargetResult.Options` as optional in the JSON tag. --- internal/target/azure_target.go | 4 + internal/target/container_target.go | 4 + internal/target/targetresult.go | 15 ++-- internal/target/targetresult_test.go | 113 +++++++++++++++++++++++++++ internal/target/vmware_target.go | 4 + 5 files changed, 135 insertions(+), 5 deletions(-) create mode 100644 internal/target/targetresult_test.go diff --git a/internal/target/azure_target.go b/internal/target/azure_target.go index 34bfd56b4..be9713282 100644 --- a/internal/target/azure_target.go +++ b/internal/target/azure_target.go @@ -24,3 +24,7 @@ func (AzureTargetOptions) isTargetOptions() {} func NewAzureTarget(options *AzureTargetOptions) *Target { return newTarget(TargetNameAzure, options) } + +func NewAzureTargetResult() *TargetResult { + return newTargetResult(TargetNameAzure, nil) +} diff --git a/internal/target/container_target.go b/internal/target/container_target.go index 7bb07a813..9ec5a81c1 100644 --- a/internal/target/container_target.go +++ b/internal/target/container_target.go @@ -17,3 +17,7 @@ func (ContainerTargetOptions) isTargetOptions() {} func NewContainerTarget(options *ContainerTargetOptions) *Target { return newTarget(TargetNameContainer, options) } + +func NewContainerTargetResult() *TargetResult { + return newTargetResult(TargetNameContainer, nil) +} diff --git a/internal/target/targetresult.go b/internal/target/targetresult.go index f8338e5ae..60efdaf62 100644 --- a/internal/target/targetresult.go +++ b/internal/target/targetresult.go @@ -9,7 +9,7 @@ import ( type TargetResult struct { Name TargetName `json:"name"` - Options TargetResultOptions `json:"options"` + Options TargetResultOptions `json:"options,omitempty"` TargetError *clienterrors.Error `json:"target_error,omitempty"` } @@ -26,7 +26,7 @@ type TargetResultOptions interface { type rawTargetResult struct { Name TargetName `json:"name"` - Options json.RawMessage `json:"options"` + Options json.RawMessage `json:"options,omitempty"` TargetError *clienterrors.Error `json:"target_error,omitempty"` } @@ -36,9 +36,14 @@ func (targetResult *TargetResult) UnmarshalJSON(data []byte) error { if err != nil { return err } - options, err := UnmarshalTargetResultOptions(rawTR.Name, rawTR.Options) - if err != nil { - return err + var options TargetResultOptions + // No options may be set if there was a target error. + // In addition, some targets don't set any options. + if len(rawTR.Options) > 0 { + options, err = UnmarshalTargetResultOptions(rawTR.Name, rawTR.Options) + if err != nil { + return err + } } targetResult.Name = rawTR.Name diff --git a/internal/target/targetresult_test.go b/internal/target/targetresult_test.go new file mode 100644 index 000000000..e364ff9eb --- /dev/null +++ b/internal/target/targetresult_test.go @@ -0,0 +1,113 @@ +package target + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/osbuild/osbuild-composer/internal/worker/clienterrors" + "github.com/stretchr/testify/assert" +) + +// Test that `Filename` set in the `Target` options gets set also in the +// `Target.ExportFilename`. +// This covers the case when new worker receives a job from old composer. +// This covers the case when new worker receives a job from new composer. +func TestTargetResultUnmarshal(t *testing.T) { + testCases := []struct { + resultJSON []byte + expectedResult *TargetResult + err bool + }{ + { + resultJSON: []byte(`{"name":"org.osbuild.aws","options":{"ami":"ami-123456789","region":"eu"}}`), + expectedResult: &TargetResult{ + Name: TargetNameAWS, + Options: &AWSTargetResultOptions{ + Ami: "ami-123456789", + Region: "eu", + }, + }, + }, + { + resultJSON: []byte(`{"name":"org.osbuild.aws.s3","options":{"url":"https://example.org/image"}}`), + expectedResult: &TargetResult{ + Name: TargetNameAWSS3, + Options: &AWSS3TargetResultOptions{ + URL: "https://example.org/image", + }, + }, + }, + { + resultJSON: []byte(`{"name":"org.osbuild.gcp","options":{"image_name":"image","project_id":"project"}}`), + expectedResult: &TargetResult{ + Name: TargetNameGCP, + Options: &GCPTargetResultOptions{ + ImageName: "image", + ProjectID: "project", + }, + }, + }, + { + resultJSON: []byte(`{"name":"org.osbuild.azure.image","options":{"image_name":"image"}}`), + expectedResult: &TargetResult{ + Name: TargetNameAzureImage, + Options: &AzureImageTargetResultOptions{ + ImageName: "image", + }, + }, + }, + { + resultJSON: []byte(`{"name":"org.osbuild.koji","options":{"image_md5":"hash","image_size":123456}}`), + expectedResult: &TargetResult{ + Name: TargetNameKoji, + Options: &KojiTargetResultOptions{ + ImageMD5: "hash", + ImageSize: 123456, + }, + }, + }, + { + resultJSON: []byte(`{"name":"org.osbuild.oci","options":{"region":"eu","image_id":"image"}}`), + expectedResult: &TargetResult{ + Name: TargetNameOCI, + Options: &OCITargetResultOptions{ + Region: "eu", + ImageID: "image", + }, + }, + }, + { + resultJSON: []byte(`{"name":"org.osbuild.vmware"}`), + expectedResult: &TargetResult{ + Name: TargetNameVMWare, + }, + }, + // target results with error without options + { + resultJSON: []byte(`{"name":"org.osbuild.aws","target_error":{"id":11,"reason":"failed to uplad image","details":["detail"]}}`), + expectedResult: &TargetResult{ + Name: TargetNameAWS, + TargetError: clienterrors.WorkerClientError(clienterrors.ErrorUploadingImage, "failed to uplad image", "detail"), + }, + }, + // unknown target name + { + resultJSON: []byte(`{"name":"org.osbuild.made.up.target","options":{}}`), + err: true, + }, + } + + for idx, testCase := range testCases { + t.Run(fmt.Sprintf("Case #%d", idx), func(t *testing.T) { + gotResult := TargetResult{} + err := json.Unmarshal(testCase.resultJSON, &gotResult) + if testCase.err { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.EqualValues(t, testCase.expectedResult, &gotResult) + } + }) + } +} diff --git a/internal/target/vmware_target.go b/internal/target/vmware_target.go index d14fee780..84153d0fe 100644 --- a/internal/target/vmware_target.go +++ b/internal/target/vmware_target.go @@ -17,3 +17,7 @@ func (VMWareTargetOptions) isTargetOptions() {} func NewVMWareTarget(options *VMWareTargetOptions) *Target { return newTarget(TargetNameVMWare, options) } + +func NewVMWareTargetResult() *TargetResult { + return newTargetResult(TargetNameVMWare, nil) +}