Repository: redhat-openshift-ecosystem/openshift-preflight
Pull request #704 (2022-06-23 16:49:55)

Add function to get formatter by name and move default value out of the `cmd` package
This PR adds a formatters.NewByName function and reconfigures existing functions to use it. This allows us to get a formatter without having to instantiate a configuration struct. 

In addition, this moves the default formatter definitions to the formatters package, and adds a test to make sure the value never fails to return a formatter. This is to help prevent dependency cycles in this test, and also makes somewhat more logical sense than storing it in the `cmd` package.

Related: #620

Signed-off-by: Jose R. Gonzalez <jose@flutes.dev>

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/704#" 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":[]} -->
coveralls: 
[![Coverage Status](https://coveralls.io/builds/50307265/badge)](https://coveralls.io/builds/50307265)

Coverage increased (+0.03%) to 61.7% when pulling **d98c7e2b61c6af8ca30cf08fe1abc64445b1476a on komish:get-format-by-name** into **d4305e841e29fdfdb013cc98d544e5f657e44ce9 on redhat-openshift-ecosystem:main**.

dcibot: -  SUCCESS https://www.distributed-ci.io/jobs/74eab254-74a1-4914-8045-5bdfd353023a/jobStates
 from https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/704
dcibot: -  SUCCESS https://www.distributed-ci.io/jobs/e4038faa-4a9a-4e1b-b9c2-3ea9ae76dfee/jobStates
 from https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/704

Patch:
diff --git a/certification/formatters/defaults.go b/certification/formatters/defaults.go
new file mode 100644
index 0000000..aaf6876
--- /dev/null
+++ b/certification/formatters/defaults.go
@@ -0,0 +1,5 @@
+package formatters
+
+const (
+	DefaultFormat = "json"
+)
diff --git a/certification/formatters/errors.go b/certification/formatters/errors.go
new file mode 100644
index 0000000..5c138c8
--- /dev/null
+++ b/certification/formatters/errors.go
@@ -0,0 +1,5 @@
+package formatters
+
+import "errors"
+
+var ErrUnknownFormatter = errors.New("requested format is unknown")
diff --git a/certification/formatters/formatters.go b/certification/formatters/formatters.go
index bceeeae..a32d999 100644
--- a/certification/formatters/formatters.go
+++ b/certification/formatters/formatters.go
@@ -30,11 +30,17 @@ type FormatterFunc = func(context.Context, runtime.Results) (response []byte, fo
 // NewForConfig returns a new formatter based on the user-provided configuration. It relies
 // on config values which should align with known/supported/built-in formatters.
 func NewForConfig(cfg certification.Config) (ResponseFormatter, error) {
-	formatter, defined := availableFormatters[cfg.ResponseFormat()]
+	return NewByName(cfg.ResponseFormat())
+}
+
+// NewByName returns a predefined ResponseFormatter with the given name.
+// TODO: New* funcs in this package may benefit from renaming.
+func NewByName(name string) (ResponseFormatter, error) {
+	formatter, defined := availableFormatters[name]
 	if !defined {
-		return nil, fmt.Errorf(
-			"failed to create a new formatter from config: %s",
-			cfg.ResponseFormat(),
+		return nil, fmt.Errorf("%w: %s",
+			ErrUnknownFormatter,
+			name,
 		)
 	}
 
diff --git a/certification/pyxis/builder.go b/certification/pyxis/builder.go
index 121b611..a20b8fe 100644
--- a/certification/pyxis/builder.go
+++ b/certification/pyxis/builder.go
@@ -13,7 +13,7 @@ import (
 // certificationInputBuilder facilitates the building of CertificationInput for
 // submitting an asset to Pyxis.
 type certificationInputBuilder struct {
-	certificationInput
+	CertificationInput
 }
 
 // NewCertificationInput accepts required values for submitting to Pyxis, and returns a CertificationInputBuilder for
@@ -25,7 +25,7 @@ func NewCertificationInput(project *CertProject) (*certificationInputBuilder, er
 	}
 
 	b := certificationInputBuilder{
-		certificationInput: certificationInput{
+		CertificationInput: CertificationInput{
 			CertProject: project,
 		},
 	}
@@ -39,7 +39,7 @@ func NewCertificationInput(project *CertProject) (*certificationInputBuilder, er
 // unmodifiable CertificationInput.
 //
 // If any required values are not included, an error is thrown.
-func (b *certificationInputBuilder) Finalize() (*certificationInput, error) {
+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")
@@ -61,7 +61,7 @@ func (b *certificationInputBuilder) Finalize() (*certificationInput, error) {
 	b.CertImage.ISVPID = b.CertProject.Container.ISVPID
 	b.CertImage.Certified = b.TestResults.Passed
 
-	return &b.certificationInput, nil
+	return &b.CertificationInput, nil
 }
 
 // WithCertImage adds a pyxis.CertImage from the passed io.Reader to the CertificationInput.
diff --git a/certification/pyxis/submit.go b/certification/pyxis/submit.go
index a3e74a2..e1d44de 100644
--- a/certification/pyxis/submit.go
+++ b/certification/pyxis/submit.go
@@ -12,7 +12,7 @@ var defaultRegistryAlias = "docker.io"
 
 // 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 2e74e65..27fe87a 100644
--- a/certification/pyxis/types.go
+++ b/certification/pyxis/types.go
@@ -6,7 +6,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 8a0a377..7729b58 100644
--- a/cmd/check_container.go
+++ b/cmd/check_container.go
@@ -64,7 +64,7 @@ func checkContainerRunE(cmd *cobra.Command, args []string) error {
 
 	// Set our runtime defaults.
 	cfg.Image = containerImage
-	cfg.ResponseFormat = DefaultOutputFormat
+	cfg.ResponseFormat = formatters.DefaultFormat
 
 	// Run the  container check.
 	cmd.SilenceUsage = true
diff --git a/cmd/check_operator.go b/cmd/check_operator.go
index d1a1330..7274610 100644
--- a/cmd/check_operator.go
+++ b/cmd/check_operator.go
@@ -56,7 +56,7 @@ func checkOperatorRunE(cmd *cobra.Command, args []string) error {
 	}
 
 	cfg.Image = operatorImage
-	cfg.ResponseFormat = DefaultOutputFormat
+	cfg.ResponseFormat = formatters.DefaultFormat
 	cfg.Bundle = true
 	cfg.Scratch = true
 
diff --git a/cmd/defaults.go b/cmd/defaults.go
index d71dd7e..813d749 100644
--- a/cmd/defaults.go
+++ b/cmd/defaults.go
@@ -1,7 +1,6 @@
 package cmd
 
 var (
-	DefaultOutputFormat      = "json"
 	DefaultLogFile           = "preflight.log"
 	DefaultLogLevel          = "info"
 	DefaultNamespace         = "default"


Test patch:
diff --git a/certification/formatters/formatters_test.go b/certification/formatters/formatters_test.go
index 41eb38a..2de88e7 100644
--- a/certification/formatters/formatters_test.go
+++ b/certification/formatters/formatters_test.go
@@ -10,6 +10,12 @@ import (
 )
 
 var _ = Describe("Formatters", func() {
+	Describe("When getting the formatter for the named default format", func() {
+		It("should never fail", func() {
+			_, err := NewByName(DefaultFormat)
+			Expect(err).ToNot(HaveOccurred())
+		})
+	})
 	Describe("When getting a new formatter for a configuration", func() {
 		Context("with a valid configuration", func() {
 			cfg := runtime.Config{
diff --git a/certification/pyxis/builder_test.go b/certification/pyxis/builder_test.go
index 7f4fb63..e9d8f3d 100644
--- a/certification/pyxis/builder_test.go
+++ b/certification/pyxis/builder_test.go
@@ -60,7 +60,7 @@ var _ = Describe("Pyxis Builder tests", func() {
 			})
 
 			It("should return a certification input builder with the project embedded", func() {
-				Expect(builder.certificationInput.CertProject.ID).To(Equal(p.ID))
+				Expect(builder.CertificationInput.CertProject.ID).To(Equal(p.ID))
 			})
 
 			It("should fail to finalize with no cert image", func() {
diff --git a/certification/pyxis/submit_test.go b/certification/pyxis/submit_test.go
index d0005bc..65e8811 100644
--- a/certification/pyxis/submit_test.go
+++ b/certification/pyxis/submit_test.go
@@ -12,7 +12,7 @@ var _ = Describe("Pyxis Submit", func() {
 	ctx := context.Background()
 
 	var pyxisClient *pyxisClient
-	var certInput certificationInput
+	var certInput CertificationInput
 	mux := http.NewServeMux()
 
 	// These go from most explicit to least explicit. They will be check that way by the ServeMux.
@@ -29,7 +29,7 @@ var _ = Describe("Pyxis Submit", func() {
 			"my-awesome-project-id",
 			&http.Client{Transport: localRoundTripper{handler: mux}},
 		)
-		certInput = certificationInput{
+		certInput = CertificationInput{
 			CertProject: &CertProject{CertificationStatus: "Started"},
 			CertImage: &CertImage{
 				Repositories: []Repository{