Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor commands to take a viper instance #1200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bcrochet
Copy link
Contributor

Instead of relying on a global instance, the commands should instead have viper injected. This will make testing easier.

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 <brad@redhat.com>
Copy link

openshift-ci bot commented Sep 20, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2024
@bcrochet
Copy link
Contributor Author

Replaces #958

@dcibot
Copy link

dcibot commented Sep 20, 2024

Copy link
Contributor

@komish komish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine overall. Mostly nits, except the ResultSubmitter{}.Submit interface definition change.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we either:

  1. 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
  2. Update the documentation such that it's clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be a type alias to make it clearer. TBH, we shouldn't even be exporting interfaces in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A type alias isn't strictly necessary. Something like this should be plenty:

Suggested change
Submit(context.Context, string) error
Submit(ctx context.Context, pyxisEnv string) error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused by this as well, it took me till the end of the PR to understand that this was pyxisEnv.

cmd/preflight/cmd/check_operator.go Show resolved Hide resolved
Comment on lines +242 to +253
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
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extraneous?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be vestigial from an earlier revision, given the age of this patch.

I'll check it out.

@bcrochet bcrochet added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2024
@@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does the same thing, but "reads" better. In both the code and the output when a test fails.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2024
@openshift-merge-robot
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants