Repository: redhat-openshift-ecosystem/openshift-preflight
Pull request #556 (2022-04-08 17:46:23)

Convert submission logic to use a builder struct
This is mostly a proof of concept, and will also potentially need a rebase when #555 merges. Until then, this will look like it contains the changes in #555 because this PR depends on it.

### Motivation:

Our `check_container.go`'s submit workflow is a stream of tasks that summarizes to "building a data structure that we can send to pyxis". Subjectively, it seems like it might read better with a more fluent flow. 

This introduces a type `pyxis.CertificationInputBuilder` type that integrates the logic we need to build our Pyxis container submission data into something a bit more fluent. It's not a perfect builder, as we end up doing some necessary error checking that makes it a bit more difficult to be fully fluent, but it gets part of the way there.

### Changes, by file

**certification/pyxis/types.go**

- Adds the `pyxis.CertificationInputBuilder` type, which contains methods that compartmentalize the toil of "read file, unmarshal, embed" which we do for several types. 
- Adds an initializer of `pyxis.CertificationInputBuilder` that we can use to enforce required values. The only value that didn't _seem_ required was Artifacts, so that instead has...
- Adds a method to allow reading multiple artifacts from disk and appending them to the Input's artifacts slice.
- Adds private methods for this type to facilitate handling all of the required items
- Adds a Finalize method which must be called in order to  return the CertificationInput that can be passed to `pyxisClient.SubmitResults`.
- I didn't see a reason to keep CertificationInput as a public type, so it's now private.

**cmd/check_container.go**

- Most of the logic in the submit block has been moved to the builder above, so the code here performs the same actions using the builder (which means less code here, because it was moved over there).

---
Using `make test`, and in my local test against UAT, it seems to work as I expect.


openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/556#" title="Author self-approved">komish</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-openshift-ecosystem%2Fopenshift-preflight).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/OWNERS)~~ [komish]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
komish: /do-not-merge

Checking the git log to make sure this only reflects the right changes now that #555 has merged.
komish: /hold
komish: /hold cancel

This should be rebased and good for review

Patch:
diff --git a/certification/pyxis/builder.go b/certification/pyxis/builder.go
new file mode 100644
index 0000000..3e4cf55
--- /dev/null
+++ b/certification/pyxis/builder.go
@@ -0,0 +1,222 @@
+package pyxis
+
+import (
+	"encoding/base64"
+	"encoding/json"
+	"fmt"
+	"io"
+	"net/http"
+	"os"
+	"path"
+
+	log "github.com/sirupsen/logrus"
+
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/errors"
+)
+
+// certificationInputBuilder facilitates the building of CertificationInput for
+// submitting an asset to Pyxis.
+type certificationInputBuilder struct {
+	certificationInput
+}
+
+// NewCertificationInput accepts required values for submitting to Pyxis, and returns a CertificationInputBuilder for
+// adding additional files as artifacts to the submission. The caller must call Finalize() in order to receive
+// a *CertificationInput.
+func NewCertificationInput(project *CertProject) (*certificationInputBuilder, error) {
+	if project == nil {
+		return nil, fmt.Errorf("a certification project was not provided and is required")
+	}
+
+	b := certificationInputBuilder{
+		certificationInput: certificationInput{
+			CertProject: project,
+		},
+	}
+
+	return &b, nil
+}
+
+// Finalize runs a collection of safeguards to try to ensure we get a reliable
+// CertificationInput. This also wires up information that's shared across
+// the various included assets (e.g. ISVPID) where applicable, and returns an
+// unmodifiable CertificationInput.
+//
+// If any required values are not included, an error is thrown.
+func (b *certificationInputBuilder) Finalize() (*certificationInput, error) {
+	// safeguards, make sure things aren't nil for any reason.
+	if b.CertImage == nil {
+		return nil, fmt.Errorf("a CertImage was not provided and is required")
+	}
+	if b.TestResults == nil {
+		return nil, fmt.Errorf("test results were not provided and are required")
+	}
+
+	if b.RpmManifest == nil {
+		return nil, fmt.Errorf("the RPM manifest was not provided and is required")
+	}
+
+	if b.Artifacts == nil {
+		// we assume artifacts can be empty, but not nil.
+		b.Artifacts = []Artifact{}
+	}
+
+	// connect values from different components as necessary.
+	b.CertImage.ISVPID = b.CertProject.Container.ISVPID
+	b.CertImage.Certified = b.TestResults.Passed
+
+	return &b.certificationInput, nil
+}
+
+// WithCertImageFromFile adds a pyxis.CertImage from a file on disk to the CertificationInput.
+// Errors are logged, but will not halt execution.
+func (b *certificationInputBuilder) WithCertImageFromFile(filepath string) *certificationInputBuilder {
+	if err := b.storeCertImage(filepath); err != nil {
+		log.Error(err)
+		return b
+	}
+
+	return b
+}
+
+// WithPreflightResultsFromFile adds formatters.UserResponse from a file on disk to the CertificationInput.
+// Errors are logged, but will not halt execution.
+func (b *certificationInputBuilder) WithPreflightResultsFromFile(filepath string) *certificationInputBuilder {
+	if err := b.storePreflightResults(filepath); err != nil {
+		log.Error(err)
+		return b
+	}
+
+	return b
+}
+
+// WithPreflightResultsFromFile adds the pyxis.RPMManifest from a file on disk to the CertificationInput.
+// Errors are logged, but will not halt execution.
+func (b *certificationInputBuilder) WithRPMManifestFromFile(filepath string) *certificationInputBuilder {
+	if err := b.storeRPMManifest(filepath); err != nil {
+		log.Error(err)
+		return b
+	}
+
+	return b
+}
+
+// WithArtifactFromFile reads a file at path and binds it as an artifact to include
+// in the submission. Multiple calls to this will append artifacts. Errors are logged,
+// but will not halt execution.
+func (b *certificationInputBuilder) WithArtifactFromFile(filepath string) *certificationInputBuilder {
+	file, err := os.Open(filepath)
+	if err != nil {
+		log.Error(err)
+		return b
+	}
+	defer file.Close()
+
+	bytes, err := io.ReadAll(file)
+	if err != nil {
+		log.Error(err)
+		return b
+	}
+
+	info, err := file.Stat()
+	if err != nil {
+		log.Error(err)
+		return b
+	}
+
+	newArtifact := Artifact{
+		CertProject: b.CertProject.ID,
+		Content:     base64.StdEncoding.EncodeToString(bytes),
+		ContentType: http.DetectContentType(bytes),
+		Filename:    path.Base(filepath),
+		FileSize:    info.Size(),
+	}
+
+	b.Artifacts = append(b.Artifacts, newArtifact)
+
+	return b
+}
+
+// storeRPMManifest reads the manifest from disk at path and stores it in
+// the CertificationInput as an RPMManifest struct.
+func (b *certificationInputBuilder) storeRPMManifest(filepath string) error {
+	bytes, err := os.ReadFile(filepath)
+	if err != nil {
+		return fmt.Errorf(
+			"%w: unable to read file from disk to include in submission: %s: %s",
+			errors.ErrSubmittingToPyxis,
+			filepath,
+			err,
+		)
+	}
+
+	var manifest RPMManifest
+	err = json.Unmarshal(bytes, &manifest)
+	if err != nil {
+		return fmt.Errorf(
+			"%w: data for the %s appears to be malformed: %s",
+			errors.ErrSubmittingToPyxis,
+			"rpm manifest",
+			err,
+		)
+	}
+
+	b.RpmManifest = &manifest
+	return nil
+}
+
+// storePreflightResults reads the results from disk at path and stores it in
+// the CertificationInput as TestResults.
+func (b *certificationInputBuilder) storePreflightResults(filepath string) error {
+	bytes, err := os.ReadFile(filepath)
+	if err != nil {
+		return fmt.Errorf(
+			"%w: unable to read file from disk to include in submission: %s: %s",
+			errors.ErrSubmittingToPyxis,
+			filepath,
+			err,
+		)
+	}
+
+	var testResults TestResults
+	err = json.Unmarshal(bytes, &testResults)
+	if err != nil {
+		return fmt.Errorf(
+			"%w: data for the %s appears to be malformed: %s",
+			errors.ErrSubmittingToPyxis,
+			"preflight results.json",
+			err,
+		)
+	}
+
+	b.TestResults = &testResults
+	return nil
+}
+
+// storeCertImage reads the image from disk at path and stores it in
+// the CertificationInput as a CertImage
+func (b *certificationInputBuilder) storeCertImage(filepath string) error {
+	bytes, err := os.ReadFile(filepath)
+	if err != nil {
+		return fmt.Errorf(
+			"%w: unable to read file from disk to include in submission: %s: %s",
+			errors.ErrSubmittingToPyxis,
+			filepath,
+			err,
+		)
+	}
+
+	var image CertImage
+	err = json.Unmarshal(bytes, &image)
+	if err != nil {
+		return fmt.Errorf(
+			"%w: data for the %s appears to be malformed: %s",
+			errors.ErrSubmittingToPyxis,
+			"certImage",
+			err,
+		)
+	}
+
+	b.CertImage = &image
+	return nil
+}
diff --git a/certification/pyxis/submit.go b/certification/pyxis/submit.go
index 6378319..b02c973 100644
--- a/certification/pyxis/submit.go
+++ b/certification/pyxis/submit.go
@@ -10,7 +10,7 @@ import (
 
 // SubmitResults takes certInput and sends requests to Pyxis to create or update entries
 // based on certInput.
-func (p *pyxisClient) SubmitResults(ctx context.Context, certInput *CertificationInput) (*CertificationResults, error) {
+func (p *pyxisClient) SubmitResults(ctx context.Context, certInput *certificationInput) (*CertificationResults, error) {
 	var err error
 
 	certProject := certInput.CertProject
diff --git a/certification/pyxis/types.go b/certification/pyxis/types.go
index 771df5f..de151b0 100644
--- a/certification/pyxis/types.go
+++ b/certification/pyxis/types.go
@@ -4,7 +4,7 @@ import (
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/formatters"
 )
 
-type CertificationInput struct {
+type certificationInput struct {
 	CertProject *CertProject
 	CertImage   *CertImage
 	TestResults *TestResults
diff --git a/cmd/check_container.go b/cmd/check_container.go
index e6af8d4..5d16d5b 100644
--- a/cmd/check_container.go
+++ b/cmd/check_container.go
@@ -2,8 +2,6 @@ package cmd
 
 import (
 	"context"
-	"encoding/base64"
-	"encoding/json"
 	"fmt"
 	"io"
 	"net/http"
@@ -150,11 +148,16 @@ var checkContainerCmd = &cobra.Command{
 		if submit {
 			log.Info("preparing results that will be submitted to Red Hat")
 
+			// you must provide a project ID in order to submit.
 			if projectId == "" {
 				return errors.ErrEmptyProjectID
 			}
+
+			// establish a pyxis client.
 			apiToken := viper.GetString("pyxis_api_token")
 			pyxisClient := pyxis.NewPyxisClient(viper.GetString("pyxis_host"), apiToken, projectId, &http.Client{Timeout: 60 * time.Second})
+
+			// get the project info from pyxis
 			certProject, err := pyxisClient.GetProject(ctx)
 			if err != nil {
 				log.Error(fmt.Errorf("%w: %s", errors.ErrRetrievingProject, err))
@@ -162,6 +165,7 @@ var checkContainerCmd = &cobra.Command{
 			}
 			log.Tracef("CertProject: %+v", certProject)
 
+			// read the provided docker config
 			dockerConfigJsonFile, err := os.Open(viper.GetString("dockerConfig"))
 			if err != nil {
 				return err
@@ -172,108 +176,39 @@ var checkContainerCmd = &cobra.Command{
 			if err != nil {
 				return err
 			}
-			certProject.Container.DockerConfigJSON = string(dockerConfigJsonBytes)
-
-			// The engine writes the certified image config to disk in a Pyxis-specific format.
-			// Include that data in our submission.
-			certImageFilepath := path.Join(artifacts.Path(), certification.DefaultCertImageFilename)
-			log.Tracef("Loading certImage data from path %s", certImageFilepath)
-			certImageBytes, err := os.ReadFile(certImageFilepath)
-			if err != nil {
-				return fmt.Errorf(
-					"%w: unable to read file from disk to include in submission: %s: %s",
-					errors.ErrSubmittingToPyxis,
-					certImageFilepath,
-					err,
-				)
-			}
 
-			var certImage pyxis.CertImage
-			err = json.Unmarshal(certImageBytes, &certImage)
-			if err != nil {
-				return fmt.Errorf(
-					"%w: data for the %s appears to be malformed: %s",
-					errors.ErrSubmittingToPyxis,
-					"certImage",
-					err,
-				)
-			}
-
-			// The certification engine also writes the rpmManifest for images not based on scratch.
-			// Include that data in our submission.
-			rpmManifestFilepath := path.Join(artifacts.Path(), certification.DefaultRPMManifestFilename)
-			log.Tracef("Loading RPM manifest data from path %s", rpmManifestFilepath)
-			rpmManifestBytes, err := os.ReadFile(rpmManifestFilepath)
-			if err != nil {
-				return fmt.Errorf(
-					"%w: unable to read file from disk to include in submission: %s: %s",
-					errors.ErrSubmittingToPyxis,
-					rpmManifestFilepath,
-					err,
-				)
-			}
-
-			var rpmManifest pyxis.RPMManifest
-			err = json.Unmarshal(rpmManifestBytes, &rpmManifest)
-			if err != nil {
-				return fmt.Errorf(
-					"%w: data for the %s appears to be malformed: %s",
-					errors.ErrSubmittingToPyxis,
-					"rpm manifest",
-					err,
-				)
-			}
+			certProject.Container.DockerConfigJSON = string(dockerConfigJsonBytes)
 
-			// Include Preflight's test results in our submission. pyxis.TestResults embeds them.
-			var testResults pyxis.TestResults
-			err = json.Unmarshal(formattedResults, &testResults)
+			// prepare submission
+			submission, err := pyxis.NewCertificationInput(certProject)
 			if err != nil {
 				return fmt.Errorf(
-					"%w: data for the %s appears to be malformed: %s",
+					"%w: could not build submission with required assets: %s",
 					errors.ErrSubmittingToPyxis,
-					"preflight results.json",
 					err,
 				)
 			}
 
-			// Populate the certImage with missing values as a result of this execution.
-			certImage.ISVPID = certProject.Container.ISVPID
-			certImage.Certified = testResults.Passed
+			submission.
+				// The engine writes the certified image config to disk in a Pyxis-specific format.
+				WithCertImageFromFile(path.Join(artifacts.Path(), certification.DefaultCertImageFilename)).
+				// Include Preflight's test results in our submission. pyxis.TestResults embeds them.
+				WithPreflightResultsFromFile(path.Join(artifacts.Path(), certification.DefaultTestResultsFilename)).
+				// The certification engine writes the rpmManifest for images not based on scratch.
+				WithRPMManifestFromFile(path.Join(artifacts.Path(), certification.DefaultRPMManifestFilename)).
+				// Include the preflight execution log file.
+				WithArtifactFromFile(viper.GetString("logfile"))
 
-			// Send the preflight logfile as an artifact in our submission.
-			logFileName := viper.GetString("logfile")
-			logFileBytes, logFileSize, err := readFileAndGetSize(logFileName)
+			input, err := submission.Finalize()
 			if err != nil {
 				return fmt.Errorf(
-					"%w: unable to read file from disk to include in submission %s: %s",
+					"%w: unable to finalize data that would be sent to pyxis %s",
 					errors.ErrSubmittingToPyxis,
-					logFileName,
 					err,
 				)
 			}
 
-			logFileArtifact := pyxis.Artifact{
-				CertProject: projectId,
-				Content:     base64.StdEncoding.EncodeToString(logFileBytes),
-				ContentType: http.DetectContentType(logFileBytes),
-				Filename:    logFileName,
-				FileSize:    logFileSize,
-			}
-
-			// additional artifacts to send to Pyxis.
-			artifacts := []pyxis.Artifact{logFileArtifact}
-
-			// assemble all of the components into the expected POST input format.
-			// and submit the results.
-			certInput := &pyxis.CertificationInput{
-				CertProject: certProject,
-				CertImage:   &certImage,
-				RpmManifest: &rpmManifest,
-				TestResults: &testResults,
-				Artifacts:   artifacts,
-			}
-
-			certResults, err := pyxisClient.SubmitResults(ctx, certInput) // TODO() This pyxisClient is established outside of this block.
+			certResults, err := pyxisClient.SubmitResults(ctx, input)
 			if err != nil {
 				return fmt.Errorf("%w: %s", errors.ErrSubmittingToPyxis, err)
 			}
diff --git a/cmd/utils.go b/cmd/utils.go
index c6ca270..9b74d38 100644
--- a/cmd/utils.go
+++ b/cmd/utils.go
@@ -101,25 +101,3 @@ func convertPassedOverall(passedOverall bool) string {
 
 	return "FAILED"
 }
-
-// readFileAndGetSize opens and reads the entire file, and also
-// returns the filesize.
-func readFileAndGetSize(path string) ([]byte, int64, error) {
-	file, err := os.Open(path)
-	if err != nil {
-		return []byte{}, 0, err
-	}
-	defer file.Close()
-
-	fileBytes, err := io.ReadAll(file)
-	if err != nil {
-		return []byte{}, 0, err
-	}
-
-	info, err := file.Stat() // Pyxis needs the file size
-	if err != nil {
-		return []byte{}, 0, err
-	}
-
-	return fileBytes, info.Size(), nil
-}


Test patch:
diff --git a/certification/pyxis/submit_test.go b/certification/pyxis/submit_test.go
index f6ed7cb..8063a41 100644
--- a/certification/pyxis/submit_test.go
+++ b/certification/pyxis/submit_test.go
@@ -26,7 +26,7 @@ var _ = Describe("Pyxis Submit", func() {
 		})
 		Context("and it is not already In Progress", func() {
 			It("should switch to In Progress", func() {
-				certResults, err := pyxisClient.SubmitResults(ctx, &CertificationInput{
+				certResults, err := pyxisClient.SubmitResults(ctx, &certificationInput{
 					CertProject: &CertProject{CertificationStatus: "Started"},
 					CertImage: &CertImage{
 						Repositories: []Repository{
@@ -56,7 +56,7 @@ var _ = Describe("Pyxis Submit", func() {
 		Context("when a project is submitted", func() {
 			Context("and the client sends a bad token", func() {
 				It("should get an unauthorized", func() {
-					certResults, err := pyxisClient.SubmitResults(ctx, &CertificationInput{
+					certResults, err := pyxisClient.SubmitResults(ctx, &certificationInput{
 						CertProject: &CertProject{CertificationStatus: "Started"},
 						CertImage: &CertImage{
 							Repositories: []Repository{
@@ -84,7 +84,7 @@ var _ = Describe("Pyxis Submit", func() {
 		Context("when a project is submitted", func() {
 			Context("and the image already exists", func() {
 				It("should get a conflict and handle it", func() {
-					certResults, err := pyxisClient.SubmitResults(ctx, &CertificationInput{
+					certResults, err := pyxisClient.SubmitResults(ctx, &certificationInput{
 						CertProject: &CertProject{},
 						CertImage: &CertImage{
 							Repositories: []Repository{
@@ -115,7 +115,7 @@ var _ = Describe("Pyxis Submit", func() {
 		Context("when a project is submitted", func() {
 			Context("and the api token is invalid", func() {
 				It("should get an unauthorized result", func() {
-					certResults, err := pyxisClient.SubmitResults(ctx, &CertificationInput{
+					certResults, err := pyxisClient.SubmitResults(ctx, &certificationInput{
 						CertProject: &CertProject{CertificationStatus: "Started"},
 						CertImage: &CertImage{
 							Repositories: []Repository{
@@ -143,7 +143,7 @@ var _ = Describe("Pyxis Submit", func() {
 		Context("when a project is submitted", func() {
 			Context("and a bad token is sent to getImage and createImage is in conflict", func() {
 				It("should error", func() {
-					certResults, err := pyxisClient.SubmitResults(ctx, &CertificationInput{
+					certResults, err := pyxisClient.SubmitResults(ctx, &certificationInput{
 						CertProject: &CertProject{CertificationStatus: "Started"},
 						CertImage: &CertImage{
 							Repositories: []Repository{
@@ -171,7 +171,7 @@ var _ = Describe("Pyxis Submit", func() {
 		Context("when a project is submitted", func() {
 			Context("and the RPM manifest already exists", func() {
 				It("should retry and return success", func() {
-					certResults, err := pyxisClient.SubmitResults(ctx, &CertificationInput{
+					certResults, err := pyxisClient.SubmitResults(ctx, &certificationInput{
 						CertProject: &CertProject{},
 						CertImage: &CertImage{
 							Repositories: []Repository{
@@ -202,7 +202,7 @@ var _ = Describe("Pyxis Submit", func() {
 		Context("when a project is submitted", func() {
 			Context("and a bad token is sent to createRPMManifest", func() {
 				It("should error", func() {
-					certResults, err := pyxisClient.SubmitResults(ctx, &CertificationInput{
+					certResults, err := pyxisClient.SubmitResults(ctx, &certificationInput{
 						CertProject: &CertProject{CertificationStatus: "Started"},
 						CertImage: &CertImage{
 							Repositories: []Repository{
@@ -230,7 +230,7 @@ var _ = Describe("Pyxis Submit", func() {
 		Context("when a project is submitted", func() {
 			Context("and a bad token is sent to getRPMManifest and createRPMManifest is in conflict", func() {
 				It("should error", func() {
-					certResults, err := pyxisClient.SubmitResults(ctx, &CertificationInput{
+					certResults, err := pyxisClient.SubmitResults(ctx, &certificationInput{
 						CertProject: &CertProject{CertificationStatus: "Started"},
 						CertImage: &CertImage{
 							Repositories: []Repository{
@@ -258,7 +258,7 @@ var _ = Describe("Pyxis Submit", func() {
 		Context("when a project is submitted", func() {
 			Context("and a bad api token is sent to createTestResults", func() {
 				It("should error", func() {
-					certResults, err := pyxisClient.SubmitResults(ctx, &CertificationInput{
+					certResults, err := pyxisClient.SubmitResults(ctx, &certificationInput{
 						CertProject: &CertProject{CertificationStatus: "Started"},
 						CertImage: &CertImage{
 							Repositories: []Repository{