-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
accept diagnostics as args #6634
Conversation
@@ -9,7 +9,7 @@ source "${OS_ROOT}/hack/util.sh" | |||
source "${OS_ROOT}/hack/cmd_util.sh" | |||
os::log::install_errexit | |||
|
|||
# Cleanup cluster resources created by this test | |||
Cleanup cluster resources created by this test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where'd the #
go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where'd the # go?
fixed
|
||
os::cmd::expect_failure_and_text 'openshift ex diagnostics FakeDiagnostic --diagnostics AlsoMissing' 'No requested diagnostics are available: requested=AlsoMissing, FakeDiagnostic' | ||
os::cmd::expect_failure_and_text 'openshift ex diagnostics AnalyzeLogs --diagnostics AlsoMissing' 'Not all requested diagnostics are available: requested=AlsoMissing, AnalyzeLogs' | ||
echo "diagnostics: ok" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW there is a file for diagnostics test cmds once this merges https://github.com/openshift/origin/pull/6397/files#diff-6061a923ddc8d6dfdd6620834faccb34
@deads2k Since there doesn't seem to be a need for subcommands I think this is a good improvement - comma lists suck. In fact if we're going to go this route I'd prefer to just remove |
[test] |
removed. |
Pulled in the deprecation commit too. Any other comments? |
@stevekuznetsov ptal |
@@ -26,6 +26,8 @@ This command validates that a configuration file intended to be used for a node | |||
|
|||
valiateNodeConfigExample = ` // Validate node configuration file | |||
$ %s openshift.local.config/master/node-config.yaml` | |||
|
|||
validateNodeConfigDeprecationMessage = `This command is deprecated and will be removed. Use 'openshift ex diagnostics NodeConfigCheck --node-config=<path/to/config.yaml' instead.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
erroneous <
in example path
rebased and comments addressed. |
LGTM after rebase |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4719/) (Image: devenv-rhel7_3222) |
3b7f7a0
to
27d612f
Compare
Evaluated for origin test up to fac65be |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/340/) |
Evaluated for origin merge up to fac65be |
Merged by openshift-bot
Allows specifying diagnostic names as args, but doesn't remove the flag.
@sosiouxme @stevekuznetsov ptal