Refactor commands to take a viper instance #1200
Conversation
Instead of relying on a global instance, the commands should instead have viper injected. This will make testing easier. Signed-off-by: Brad P. Crochet <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bcrochet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
Replaces #958 |
|
from change #1200: |
| @@ -29,7 +28,7 @@ type ResultWriter interface { | |||
|
|
|||
| // ResultSubmitter defines methods associated with submitting results to Red HAt. | |||
| type ResultSubmitter interface { | |||
| Submit(context.Context) error | |||
| Submit(context.Context, string) error | |||
There was a problem hiding this comment.
Can we either:
- Name the parameters here, such that it's clear what the string value should be without having to look at when it's called, or
- Update the documentation such that it's clear?
There was a problem hiding this comment.
It could be a type alias to make it clearer. TBH, we shouldn't even be exporting interfaces in the first place.
There was a problem hiding this comment.
A type alias isn't strictly necessary. Something like this should be plenty:
| Submit(context.Context, string) error | |
| Submit(ctx context.Context, pyxisEnv string) error |
There was a problem hiding this comment.
I was confused by this as well, it took me till the end of the PR to understand that this was pyxisEnv.
| func checkContainerPreRunE(cmd *cobra.Command, args []string) error { | ||
| certificationProjectID := viper.GetString("certification_project_id") | ||
| validatedCertificationProjectID, err := validateCertificationProjectID(certificationProjectID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if validatedCertificationProjectID != certificationProjectID { | ||
| viper.Set("certification_project_id", validatedCertificationProjectID) | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Might be vestigial from an earlier revision, given the age of this patch.
I'll check it out.
bcrochet
added
the
do-not-merge/work-in-progress
| @@ -145,24 +152,24 @@ var _ = Describe("Check Container Command", func() { | |||
| Entry("index manifest, invalid platform", "index", "none", HaveOccurred(), true), | |||
| ) | |||
|
|
|||
| Context("When validating check container arguments and flags", func() { | |||
| When("validating check container arguments and flags", func() { | |||
There was a problem hiding this comment.
Non-blocking: Why did you switch from context to when, is there some benefit? I'm fine with the change, but just want to understand the driving factor.
There was a problem hiding this comment.
It does the same thing, but "reads" better. In both the code and the output when a test fails.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Instead of relying on a global instance, the commands should instead have viper injected. This will make testing easier.