Repository: redhat-openshift-ecosystem/openshift-preflight
Pull request #721 (2022-06-28 19:36:26)

fix bug in derived runtime config where values were not populated from parameter
In writing tests, I discovered a bug in the runtime.Config derivation where the configuration values were not being populated from the passed in parameter, but rather from the upstream viper instance. 

It probably would have looked the same at execution time because we still fully leverage the upstream viper early on in the process, but the tests called just the derivation functions and caught it. 

Should add 100% coverage for this source file.

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/721#" 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/50438310/badge)](https://coveralls.io/builds/50438310)

Coverage increased (+1.1%) to 71.282% when pulling **84ac45a08d6beca7e12af6cb3ed7ae585a700f57 on komish:bugfix-and-test-cfg-from-viper** into **6d5a77de5b7f5e1f2d9506adf4860aefa9a6e179 on redhat-openshift-ecosystem:main**.

dcibot: -  SUCCESS https://www.distributed-ci.io/jobs/b85ddbb3-124f-498c-a74d-b814966707e3/jobStates
 from https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/721
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/721#pullrequestreview-1022368641" title="Approved">acornett21</a>*, *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/721#" 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)~~ [acornett21,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":[]} -->
dcibot: -  SUCCESS https://www.distributed-ci.io/jobs/e4f345c6-aab7-468c-85f4-535ea6c4cf4f/jobStates
 from https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/721
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/721#pullrequestreview-1022368641" title="Approved">acornett21</a>*, *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/721#" title="Author self-approved">komish</a>*, *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/721#pullrequestreview-1023252883" title="Approved">skattoju</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)~~ [acornett21,komish,skattoju]

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":[]} -->

Patch:
diff --git a/certification/runtime/config.go b/certification/runtime/config.go
index d011b56..8338483 100644
--- a/certification/runtime/config.go
+++ b/certification/runtime/config.go
@@ -50,7 +50,7 @@ func NewConfigFrom(vcfg viper.Viper) (*Config, error) {
 	cfg.LogFile = vcfg.GetString("logfile")
 	cfg.DockerConfig = vcfg.GetString("dockerConfig")
 	cfg.Artifacts = vcfg.GetString("artifacts")
-	cfg.WriteJUnit = viper.GetBool("junit")
+	cfg.WriteJUnit = vcfg.GetBool("junit")
 	cfg.storeContainerPolicyConfiguration(vcfg)
 	cfg.storeOperatorPolicyConfiguration(vcfg)
 	return &cfg, nil
@@ -75,10 +75,10 @@ func (c *Config) storeContainerPolicyConfiguration(vcfg viper.Viper) {
 // items in viper, normalizes them, and stores them in Config.
 func (c *Config) storeOperatorPolicyConfiguration(vcfg viper.Viper) {
 	c.Kubeconfig = os.Getenv("KUBECONFIG")
-	c.Namespace = viper.GetString("namespace")
-	c.ServiceAccount = viper.GetString("serviceaccount")
-	c.ScorecardImage = viper.GetString("scorecard_image")
-	c.ScorecardWaitTime = viper.GetString("scorecard_wait_time")
-	c.Channel = viper.GetString("channel")
-	c.IndexImage = viper.GetString("indeximage")
+	c.Namespace = vcfg.GetString("namespace")
+	c.ServiceAccount = vcfg.GetString("serviceaccount")
+	c.ScorecardImage = vcfg.GetString("scorecard_image")
+	c.ScorecardWaitTime = vcfg.GetString("scorecard_wait_time")
+	c.Channel = vcfg.GetString("channel")
+	c.IndexImage = vcfg.GetString("indeximage")
 }
diff --git a/cmd/check_operator.go b/cmd/check_operator.go
index 7274610..d5a4bbc 100644
--- a/cmd/check_operator.go
+++ b/cmd/check_operator.go
@@ -3,11 +3,8 @@ package cmd
 import (
 	"context"
 	"fmt"
-	"io"
 	"os"
-	"path/filepath"
 
-	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/artifacts"
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/engine"
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/formatters"
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/policy"
@@ -22,34 +19,70 @@ var checkOperatorCmd = &cobra.Command{
 	Use:   "operator",
 	Short: "Run checks for an Operator",
 	Long:  `This command will run the Certification checks for an Operator bundle image. `,
-	Args: func(cmd *cobra.Command, args []string) error {
-		if len(args) != 1 {
-			return fmt.Errorf("an operator image positional argument is required")
-		}
-		return nil
-	},
+	Args:  checkOperatorPositionalArgs,
 	// this fmt.Sprintf is in place to keep spacing consistent with cobras two spaces that's used in: Usage, Flags, etc
 	Example: fmt.Sprintf("  %s", "preflight check operator quay.io/repo-name/operator-bundle:version"),
 	RunE:    checkOperatorRunE,
 }
 
-// checkOperatorRunE is a cobra RunE compatible function that prepares
-// the user configuration for check operator.
-func checkOperatorRunE(cmd *cobra.Command, args []string) error {
-	log.Info("certification library version ", version.Version.String())
+// checkOperatorRunner contains all of the components necessary to run checkOperator.
+type checkOperatorRunner struct {
+	cfg       *runtime.Config
+	eng       engine.CheckEngine
+	formatter formatters.ResponseFormatter
+	rw        resultWriter
+}
 
-	ctx := cmd.Context()
+// newCheckOperatorRunner returns a checkOperatorRunner containing all of the tooling necessary
+// to run checkOperator.
+func newCheckOperatorRunner(ctx context.Context, cfg *runtime.Config) (*checkOperatorRunner, error) {
+	cfg.Policy = policy.PolicyOperator
+	cfg.Submit = false // there's no such thing as submitting for operators today.
 
-	operatorImage := args[0]
+	engine, err := engine.NewForConfig(ctx, cfg.ReadOnly())
+	if err != nil {
+		return nil, err
+	}
+
+	fmttr, err := formatters.NewForConfig(cfg.ReadOnly())
+	if err != nil {
+		return nil, err
+	}
 
+	return &checkOperatorRunner{
+		cfg:       cfg,
+		eng:       engine,
+		formatter: fmttr,
+		rw:        &runtime.ResultWriterFile{},
+	}, nil
+}
+
+// ensureKubeconfigIsSet ensures that the KUBECONFIG environment variable has a value.
+func ensureKubeconfigIsSet() error {
 	if _, ok := os.LookupEnv("KUBECONFIG"); !ok {
 		return fmt.Errorf("environment variable KUBECONFIG could not be found")
 	}
 
+	return nil
+}
+
+// ensureIndexImageConfigIsSet ensures that the PFLT_INDEXIMAGE environment variable has
+// a value.
+func ensureIndexImageConfigIsSet() error {
 	if catalogImage := viper.GetString("indexImage"); len(catalogImage) == 0 {
 		return fmt.Errorf("environment variable PFLT_INDEXIMAGE could not be found")
 	}
 
+	return nil
+}
+
+// checkOperatorRunE executes checkOperator using the user args to inform the execution.
+func checkOperatorRunE(cmd *cobra.Command, args []string) error {
+	log.Info("certification library version ", version.Version.String())
+	ctx := cmd.Context()
+	operatorImage := args[0]
+
+	// Render the Viper configuration as a runtime.Config
 	cfg, err := runtime.NewConfigFrom(*viper.GetViper())
 	if err != nil {
 		return fmt.Errorf("invalid configuration: %w", err)
@@ -60,68 +93,36 @@ func checkOperatorRunE(cmd *cobra.Command, args []string) error {
 	cfg.Bundle = true
 	cfg.Scratch = true
 
-	// Run the operator check
-	cmd.SilenceUsage = true
-	return checkOperator(ctx, cfg)
-}
-
-func checkOperator(ctx context.Context, cfg *runtime.Config) error {
-	cfg.Policy = policy.PolicyOperator
-
-	// configure the artifacts directory if the user requested a different directory.
-	if cfg.Artifacts != "" {
-		artifacts.SetDir(cfg.Artifacts)
-	}
-
-	engine, err := engine.NewForConfig(ctx, cfg.ReadOnly())
-	if err != nil {
-		return err
-	}
-
-	formatter, err := formatters.NewForConfig(cfg.ReadOnly())
+	checkOperator, err := newCheckOperatorRunner(ctx, cfg)
 	if err != nil {
 		return err
 	}
 
-	// create the results file early to catch cases where we are not
-	// able to write to the filesystem before we attempt to execute checks.
-	resultsFile, err := os.OpenFile(
-		filepath.Join(artifacts.Path(), resultsFilenameWithExtension(formatter.FileExtension())),
-		os.O_WRONLY|os.O_CREATE|os.O_TRUNC,
-		0o600,
+	// Run the operator check
+	cmd.SilenceUsage = true
+	return preflightCheck(ctx,
+		checkOperator.cfg,
+		nil, // no pyxisClient is necessary
+		checkOperator.eng,
+		checkOperator.formatter,
+		checkOperator.rw,
+		&noopSubmitter{}, // we do not submit these results.
 	)
-	if err != nil {
-		return err
-	}
-
-	// also write to stdout
-	resultsOutputTarget := io.MultiWriter(os.Stdout, resultsFile)
+}
 
-	// execute the checks
-	if err := engine.ExecuteChecks(ctx); err != nil {
-		return err
+func checkOperatorPositionalArgs(cmd *cobra.Command, args []string) error {
+	if len(args) != 1 {
+		return fmt.Errorf("an operator bundle image positional argument is required")
 	}
-	results := engine.Results(ctx)
 
-	// return results to the user and then close output files
-	formattedResults, err := formatter.Format(ctx, results)
-	if err != nil {
+	if err := ensureKubeconfigIsSet(); err != nil {
 		return err
 	}
 
-	fmt.Fprint(resultsOutputTarget, string(formattedResults))
-	if err := resultsFile.Close(); err != nil {
+	if err := ensureIndexImageConfigIsSet(); err != nil {
 		return err
 	}
 
-	if cfg.WriteJUnit {
-		if err := writeJUnit(ctx, results); err != nil {
-			return err
-		}
-	}
-
-	log.Infof("Preflight result: %s", convertPassedOverall(results.PassedOverall))
-
 	return nil
 }
 


Test patch:
diff --git a/certification/runtime/config_test.go b/certification/runtime/config_test.go
new file mode 100644
index 0000000..a9d00f2
--- /dev/null
+++ b/certification/runtime/config_test.go
@@ -0,0 +1,76 @@
+package runtime
+
+import (
+	"reflect"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"github.com/spf13/viper"
+)
+
+var _ = Describe("Viper to Runtime Config", func() {
+	var baseViperCfg *viper.Viper
+	var expectedRuntimeCfg *Config
+	BeforeEach(func() {
+		baseViperCfg = viper.New()
+		expectedRuntimeCfg = &Config{}
+
+		baseViperCfg.Set("logfile", "logfile")
+		expectedRuntimeCfg.LogFile = "logfile"
+		baseViperCfg.Set("dockerConfig", "dockerConfig")
+		expectedRuntimeCfg.DockerConfig = "dockerConfig"
+		baseViperCfg.Set("artifacts", "artifacts")
+		expectedRuntimeCfg.Artifacts = "artifacts"
+		baseViperCfg.Set("junit", true)
+		expectedRuntimeCfg.WriteJUnit = true
+
+		baseViperCfg.Set("pyxis_api_token", "apitoken")
+		expectedRuntimeCfg.PyxisAPIToken = "apitoken"
+		baseViperCfg.Set("submit", true)
+		expectedRuntimeCfg.Submit = true
+		baseViperCfg.Set("pyxis_env", "prod")
+		expectedRuntimeCfg.PyxisHost = "catalog.redhat.com/api/containers"
+		baseViperCfg.Set("certification_project_id", "000000000000")
+		expectedRuntimeCfg.CertificationProjectID = "000000000000"
+
+		baseViperCfg.Set("namespace", "myns")
+		expectedRuntimeCfg.Namespace = "myns"
+		baseViperCfg.Set("serviceaccount", "mysa")
+		expectedRuntimeCfg.ServiceAccount = "mysa"
+		baseViperCfg.Set("scorecard_image", "myscorecardimage")
+		expectedRuntimeCfg.ScorecardImage = "myscorecardimage"
+		baseViperCfg.Set("scorecard_wait_time", "100")
+		expectedRuntimeCfg.ScorecardWaitTime = "100"
+		baseViperCfg.Set("channel", "mychannel")
+		expectedRuntimeCfg.Channel = "mychannel"
+		baseViperCfg.Set("indeximage", "myindeximage")
+		expectedRuntimeCfg.IndexImage = "myindeximage"
+	})
+
+	Context("With values in a viper config", func() {
+		It("should populate a runtime.Config with container and operator policy values", func() {
+			cfg, err := NewConfigFrom(*baseViperCfg)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(*cfg).To(BeEquivalentTo(*expectedRuntimeCfg))
+		})
+	})
+
+	Context("With proper values, but with an opsid- prefix on the certification project ID", func() {
+		It("should properly trim the ospid- prefix and succeed", func() {
+			baseViperCfg.Set("certification_project_id", "ospid-111111111111")
+			expectedRuntimeCfg.CertificationProjectID = "111111111111"
+			cfg, err := NewConfigFrom(*baseViperCfg)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(*cfg).To(BeEquivalentTo(*expectedRuntimeCfg))
+		})
+	})
+
+	It("should only have 20 struct keys for tests to be valid", func() {
+		// If this test fails, it means a developer has added or removed
+		// keys from runtime.Config, and so these tests may no longer be
+		// accurate in confirming that the derived configuration from viper
+		// matches.
+		keys := reflect.TypeOf(Config{}).NumField()
+		Expect(keys).To(Equal(20))
+	})
+})
diff --git a/cmd/check_operator_test.go b/cmd/check_operator_test.go
new file mode 100644
index 0000000..476f2d0
--- /dev/null
+++ b/cmd/check_operator_test.go
@@ -0,0 +1,166 @@
+package cmd
+
+import (
+	"context"
+	"os"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/engine"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/formatters"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/runtime"
+	"github.com/spf13/viper"
+)
+
+var _ = Describe("Check Operator", func() {
+	Context("when running the check operator subcommand", func() {
+		Context("without the operator bundle image being provided", func() {
+			It("should return an error", func() {
+				_, err := executeCommand(rootCmd, "check", "operator")
+				Expect(err).To(HaveOccurred())
+			})
+		})
+
+		Context("without having set the KUBECONFIG environment variable", func() {
+			It("should return an error", func() {
+				out, err := executeCommand(rootCmd, "check", "operator", "quay.io/example/image:mytag")
+				Expect(err).To(HaveOccurred())
+				Expect(out).To(ContainSubstring("KUBECONFIG could not"))
+			})
+		})
+
+		Context("without having set the PFLT_INDEXIMAGE environment variable", func() {
+			BeforeEach(func() { os.Setenv("KUBECONFIG", "foo") })
+			AfterEach(func() { os.Unsetenv("KUBECONFIG") })
+			It("should return an error", func() {
+				out, err := executeCommand(rootCmd, "check", "operator", "quay.io/example/image:mytag")
+				Expect(err).To(HaveOccurred())
+				Expect(out).To(ContainSubstring("PFLT_INDEXIMAGE could not"))
+			})
+		})
+
+		Context("With all of the required parameters", func() {
+			BeforeEach(func() {
+				viper.Set("indexImage", "foo")
+				os.Setenv("KUBECONFIG", "foo")
+			})
+
+			AfterEach(func() {
+				viper.Set("indexImage", "")
+				os.Unsetenv("KUBECONFIG")
+			})
+
+			It("should reach the core logic, but throw an error because of the placeholder values", func() {
+				_, err := executeCommand(rootCmd, "check", "operator", "quay.io/example/image:mytag")
+				Expect(err).To(HaveOccurred())
+			})
+		})
+	})
+
+	Context("When checking for required environment variables", func() {
+		Context("specifically, KUBECONFIG", func() {
+			BeforeEach(func() { os.Setenv("KUBECONFIG", "foo") })
+			AfterEach(func() { os.Unsetenv("KUBECONFIG") })
+			It("should not encounter an error if the value is set", func() {
+				err := ensureKubeconfigIsSet()
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			It("should encounter an error if the value is not set", func() {
+				os.Unsetenv("KUBECONFIG")
+				err := ensureKubeconfigIsSet()
+				Expect(err).To(HaveOccurred())
+			})
+		})
+
+		Context("specifically, PFLT_INDEXIMAGE", func() {
+			BeforeEach(func() { viper.Set("indexImage", "foo") })
+			AfterEach(func() { viper.Set("indexImage", "") })
+			It("should not encounter an error if the value is set", func() {
+				err := ensureIndexImageConfigIsSet()
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			It("should encounter an error if the value is not set", func() {
+				viper.Set("indexImage", "")
+				err := ensureIndexImageConfigIsSet()
+				Expect(err).To(HaveOccurred())
+			})
+		})
+	})
+
+	Context("When instantiating a checkOperatorRunner", func() {
+		var cfg *runtime.Config
+		BeforeEach(func() {
+			cfg = &runtime.Config{
+				Image:          "quay.io/example/foo:latest",
+				ResponseFormat: formatters.DefaultFormat,
+			}
+		})
+
+		Context("with a valid policy formatter", func() {
+			It("should return with no error, and the appropriate formatter", func() {
+				cfg.ResponseFormat = "xml"
+				runner, err := newCheckOperatorRunner(context.TODO(), cfg)
+				Expect(err).ToNot(HaveOccurred())
+				expectedFormatter, err := formatters.NewByName(cfg.ResponseFormat)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(runner.formatter.PrettyName()).To(Equal(expectedFormatter.PrettyName()))
+			})
+		})
+
+		Context("with an invalid policy formatter", func() {
+			It("should return an error", func() {
+				cfg.ResponseFormat = "foo"
+				_, err := newCheckOperatorRunner(context.TODO(), cfg)
+				Expect(err).To(HaveOccurred())
+			})
+		})
+
+		Context("with an invalid policy definition", func() {
+			It("should return the container policy engine anyway", func() {
+				runner, err := newCheckOperatorRunner(context.TODO(), cfg)
+				Expect(err).ToNot(HaveOccurred())
+
+				expectedEngine, err := engine.NewForConfig(context.TODO(), cfg.ReadOnly())
+				Expect(runner.eng).To(BeEquivalentTo(expectedEngine))
+				Expect(err).ToNot(HaveOccurred())
+			})
+		})
+
+		Context("with an invalid formatter definition", func() {
+			It("should return an error", func() {
+				cfg.ResponseFormat = "foo"
+				_, err := newCheckOperatorRunner(context.TODO(), cfg)
+				Expect(err).To(HaveOccurred())
+			})
+		})
+
+		It("should contain a ResultWriterFile resultWriter", func() {
+			runner, err := newCheckOperatorRunner(context.TODO(), cfg)
+			Expect(err).ToNot(HaveOccurred())
+			_, rwIsExpectedType := runner.rw.(*runtime.ResultWriterFile)
+			Expect(rwIsExpectedType).To(BeTrue())
+		})
+	})
+
+	Context("When testing positional arg parsing", func() {
+		// failure cases are tested earlier in this file by running executeCommand.
+		// This tests the success case using the standalone function in order
+		// to prevent trying to run the entire RunE func in previous cases.
+		posArgs := []string{"firstparam"}
+		BeforeEach(func() {
+			viper.Set("indexImage", "foo")
+			os.Setenv("KUBECONFIG", "foo")
+		})
+
+		AfterEach(func() {
+			viper.Set("indexImage", "")
+			os.Unsetenv("KUBECONFIG")
+		})
+		It("should succeed when all positional arg constraints and environment constraints are correct", func() {
+			err := checkOperatorPositionalArgs(checkOperatorCmd, posArgs)
+			Expect(err).ToNot(HaveOccurred())
+		})
+	})
+})