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.

Sign up for GitHub

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

Jump to bottom

cmd/openshift-install: Stricter argument parsing #1125

Merged
merged 2 commits into from
Jan 26, 2019
Merged

cmd/openshift-install: Stricter argument parsing #1125

merged 2 commits into from
Jan 26, 2019

Conversation

wking
Copy link
Member

@wking wking commented Jan 24, 2019

If we can't parse the value, wait until we've setup stderr logging before complaining about it. With this pull-request:

$ openshift-install --log-level destroy create cluster
FATAL invalid log-level: not a valid logrus Level: "destroy"
$ echo $?
1

while previously:

$ openshift-install --log-level destroy create cluster
$ echo $?
1

@wking
cmd/openshift-install/main: Fix invalid-log-level warnings
772b514 
If we can't parse the value, wait until we've setup stderr logging
before complaining about it.  With this commit:

  $ openshift-install --log-level destroy create cluster
  FATAL invalid log-level: not a valid logrus Level: "destroy"
  $ echo $?
  1

while previously:

  $ openshift-install --log-level destroy create cluster
  $ echo $?
  1
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 24, 2019
@wking
Copy link
Member Author

wking commented Jan 24, 2019

I've pushed a0c48e4 to also check for invalid extra positional arguments for openshift-install create ${TARGET}.

staebler
staebler reviewed Jan 24, 2019
View reviewed changes
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

Looks good.

cmd/openshift-install/create.go Show resolved Hide resolved
abhinavdahiya
abhinavdahiya reviewed Jan 24, 2019
View reviewed changes
cmd/openshift-install/main.go
}
cmd.PersistentFlags().StringVar(&rootOpts.dir, "dir", ".", "assets directory")
cmd.PersistentFlags().StringVar(&rootOpts.logLevel, "log-level", "info", "log level (e.g. \"debug | info | warn | error\")")
return cmd
}

func runRootCmd(cmd *cobra.Command, args []string) error {
func runRootCmd(cmd *cobra.Command, args []string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ques: wouldn't just performing the level parsing check before setting logrus.SetOutput(ioutil.Discard) provide resolution to problem

If we can't parse the value, wait until we've setup stderr logging
before complaining about it.  With this commit:

  $ openshift-install --log-level destroy create cluster
  FATAL invalid log-level: not a valid logrus Level: "destroy"
  $ echo $?
  1

while previously:

  $ openshift-install --log-level destroy create cluster
  $ echo $?
  1

Copy link
Member Author

Choose a reason for hiding this comment

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

ques: wouldn't just performing the level parsing check before setting logrus.SetOutput(ioutil.Discard) provide resolution to problem

Sort of. It would land before our stderr hook and its &logrus.TextFormatter config, so the output looks like:

$ openshift-install --log-level destroy create cluster
FATA[0000] invalid log-level: not a valid logrus Level: "destroy"

My weak preference is for 772b514 as it stands, but I can switch this around if you'd prefer the earlier level parser approach.

@abhinavdahiya
Copy link
Contributor

Also 5133169 looks out of place in this PR based on the title, can we update the PR desciption and title to reflect that? It is fine to get both these in, in one PR with CI flaking... :(

@wking wking changed the title cmd/openshift-install/main: Fix invalid-log-level warnings cmd/openshift-install: Stricter argument paraing Jan 25, 2019
@smarterclayton
Copy link
Contributor

/refresh

@wking wking changed the title cmd/openshift-install: Stricter argument paraing cmd/openshift-install: Stricter argument parsing Jan 25, 2019
@wking
Copy link
Member Author

wking commented Jan 25, 2019

Still lots of "Expected" for Prow.

/refresh

@wking
Copy link
Member Author

wking commented Jan 25, 2019

Prow was still stuck, so I've bumped the commit date with 5133169 -> 5fedd84 to unstick it.

@wking
cmd/openshift-install: Error on additional arguments
a8c1aa3 
With this commit:

  $ openshift-install create install-config whatever
  FATA[0000] Error executing openshift-install: accepts 0 arg(s), received 1
  $ echo $?
  1

instead of silently ignoring "whatever" and creating the
install-config.  Docs in [1,2].

[1]: https://godoc.org/github.com/spf13/cobra#ExactArgs
[2]: https://godoc.org/github.com/spf13/cobra#Command
@wking
Copy link
Member Author

wking commented Jan 25, 2019

I've pushed 5fedd84 -> a8c1aa3 fixing the gofmt errors.

@wking wking added this to the 0.11.0 milestone Jan 25, 2019
@wking
Copy link
Member Author

wking commented Jan 25, 2019

e2e-aws:

level=info msg="Waiting up to 10m0s for the openshift-console route to be created..."
level=fatal msg="waiting for openshift-console URL: context deadline exceeded"

/retest

@wking
Copy link
Member Author

wking commented Jan 25, 2019

e2e-aws:

2019/01/25 21:20:42 Running pod e2e-aws
level=fatal msg="failed to fetch Terraform Variables: failed to fetch dependency of \"Terraform Variables\": failed to generate asset \"Image\": failed to fetch RHCOS metadata: failed to fetch latest build: incorrect HTTP response (503 Service Unavailable)"

/retest

@wking
Copy link
Member Author

wking commented Jan 25, 2019

e2e-aws:

Flaky tests:

[sig-apps] Deployment iterative rollouts should eventually progress [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-scheduling] ResourceQuota should create a ResourceQuota and capture the life of a secret. [Suite:openshift/conformance/parallel] [Suite:k8s]

Failing tests:

[Feature:DeploymentConfig] deploymentconfigs when run iteratively [Conformance] should immediately start a new deployment [Suite:openshift/conformance/parallel/minimal]
[sig-storage] Volume limits should verify that all nodes have volume limits [Suite:openshift/conformance/parallel] [Suite:k8s]

/retest

@crawford
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, wking

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

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@wking
Copy link
Member Author

wking commented Jan 26, 2019

/retest

Pick up openshift/origin#21867.

@openshift-merge-robot openshift-merge-robot merged commit 0098511 into openshift:master Jan 26, 2019
@wking wking deleted the invalid-log-level-errors branch January 27, 2019 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@staebler staebler staebler left review comments

@rajatchopra rajatchopra Awaiting requested review from rajatchopra

@steveej steveej Awaiting requested review from steveej

Assignees

@crawford crawford

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
0.11.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@wking @abhinavdahiya @smarterclayton @crawford @openshift-ci-robot @openshift-bot @staebler @openshift-merge-robot