diagnostics: fix various issues #11295
Conversation
sosiouxme
force-pushed
the
20161010-diagnostics-fixes
branch
2 times, most recently
from
a744e80 to
b14e6f1
Compare
|
@jcantrill before i forget again to link you on this I still have at least another thing or two I'd like to add |
|
|
||
| Use the 'oadm policy reconcile-cluster-roles' command to create the role. For example, | ||
|
|
||
| $ oadm policy reconcile-cluster-roles \ |
There was a problem hiding this comment.
For consistency we should probably update all places to include '$' in the other messages found in AGL diagnostics
There was a problem hiding this comment.
The CLI Hacking Guide says no $:
https://github.com/openshift/origin/blob/master/docs/cli_hacking_guide.adoc#writing-examples
And that's what most if not all commands follow.
Wouldn't it be the case for oc diagnostics too?
There was a problem hiding this comment.
I don't see where it explicitly says to not use dollar signs but I think this example is subtly different. Here we are suggesting a command you can run to resolve a discovered issue. We are not suggesting an example of running a diagnostics command. Regardless, I suggest we either add '$' or remove them to be consistent with places we have used them before.
There was a problem hiding this comment.
We removed all $ prefixes back in May to follow the style in Kubernetes: #8454.
I'm not sure if it's the case here, but for the CLI commands those comments become part of the generated docs, so I agree with you that we need to be consistent.
There was a problem hiding this comment.
I'm not sure if it's the case here,
Most probably not the case... these seem to be just constants that I believe are printed as output of the diagnostics.
There was a problem hiding this comment.
I could argue that bit of the CLI hacking guide is only about the command usage statement. I think diagnostics is the only place that has output related to running other commands entirely. It's a gray area; I think it is clearer with the $ to indicate commands; what do you think?
bug 1382903 https://bugzilla.redhat.com/show_bug.cgi?id=1382903 When the LoggingPublicUrl isn't configured in master config, the diagnostic no longer silently succeeds, but instead is skipped with a message about why.
sosiouxme
force-pushed
the
20161010-diagnostics-fixes
branch
from
b14e6f1 to
ebceafe
Compare
for consistency, the commands we tell the user to follow are all preceded by a shell prompt.
|
[merge] |
|
Evaluated for origin merge up to 9b82a96 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11610/) (Base Commit: 826c379) (Image: devenv-rhel7_5394) |
Addresses https://bugzilla.redhat.com/show_bug.cgi?id=1382903 and https://bugzilla.redhat.com/show_bug.cgi?id=1381611