Skip to content

Edits after #8644 #10983

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
Merged
merged 1 commit into from
Jul 23, 2018
Merged

Edits after #8644 #10983

merged 1 commit into from
Jul 23, 2018

Conversation

bfallonf
Copy link

@bfallonf bfallonf commented Jul 23, 2018 •
edited
Loading

For: #8644

cc @vikram-redhat @rlopez133

@openshift/team-documentation PTAL

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 23, 2018
@openshift-docs-preview-bot
Copy link

The preview will be availble shortly at:

@bfallonf bfallonf added the peer-review-needed Signifies that the peer review team needs to review this PR label Jul 23, 2018
@bfallonf bfallonf mentioned this pull request Jul 23, 2018
Closed
kalexand-rh
kalexand-rh reviewed Jul 23, 2018
View reviewed changes
Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

I have a couple of suggestions and a request. This sounds good. :)

admin_guide/diagnostics_tool.adoc
@@ -50,7 +49,7 @@ administrator, run:
# oc adm diagnostics
----

This runs all available diagnostics, skipping any that do not apply.
This runs all available diagnostics and skips any that do not apply to the environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

I might s/this runs/this command starts

admin_guide/diagnostics_tool.adoc Outdated
operating. The diagnostics attempt to use as much access as the user has
available.
The diagnostics tool runs using the level of permissions granted to the
account from which you run it. For example, as an ordinary user or a `cluster-admin` user.
Copy link
Contributor

Choose a reason for hiding this comment

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

"For example, as an ordinary user or a cluster-admin user. " is a fragment. I think you can remove it without impacting the meaning of this section. Please do that or work it into the previous sentence.

admin_guide/diagnostics_tool.adoc Outdated
same inventory file used for deployment. The changes consist of installing
dependencies so that the checks can gather the required information. In some
circumstances, additional system components, such as `docker` or networking
configurations can be altered if their current state differs from the
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd s/can be altered/are changed

admin_guide/diagnostics_tool.adoc Outdated
dependencies so that the checks can gather the required information. In some
circumstances, additional system components, such as `docker` or networking
configurations can be altered if their current state differs from the
configuration in the inventory file. You should run these health checks only if
Copy link
Contributor

Choose a reason for hiding this comment

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

s/You should run/Run

admin_guide/diagnostics_tool.adoc Outdated
As a non-root user that has privileges to run containers specify the cluster's
inventory file and the *_health.yml_* playbook:
Run the following as a non-root user that has privileges to run containers and
specify the cluster's inventory file and the *_health.yml_* playbook:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to say "specify the cluster's inventory file and the health.yml playbook" here? Would adding an annotation to line 383 be better? If we do, I'd s/specify/specify both

admin_guide/diagnostics_tool.adoc Outdated
* Allowing a user to provide a *_known_hosts_* file and have SSH validate host
keys. This is disabled by the default configuration and can be re-enabled with
an environment variable by adding `-e ANSIBLE_HOST_KEY_CHECKING=True` to the
`docker` command line.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an extra space here?

@kalexand-rh kalexand-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Jul 23, 2018
@bfallonf
Copy link
Author

Thanks @kalexand-rh . Merging.

@bfallonf bfallonf merged commit a35ce7b into openshift:master Jul 23, 2018
@bfallonf
Copy link
Author

/cherrypick enterprise-3.10

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jul 23, 2018
Merged
@openshift-cherrypick-robot
Copy link

@bfallonf: new pull request created: #11012

In response to this:

/cherrypick enterprise-3.10

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/test-infra repository.

@bfallonf
Copy link
Author

/cherrypick enterprise-3.9

@openshift-cherrypick-robot
Copy link

@bfallonf: new pull request created: #11013

In response to this:

/cherrypick enterprise-3.9

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/test-infra repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jul 24, 2018
Merged
@bfallonf
Copy link
Author

/cherrypick enterprise-3.7

@openshift-cherrypick-robot
Copy link

@bfallonf: #10983 failed to apply on top of branch "enterprise-3.7":

.git/rebase-apply/patch:58: trailing whitespace.
An Ansible-deployed cluster provides additional diagnostic benefits for 
.git/rebase-apply/patch:122: trailing whitespace.
configuration. 
.git/rebase-apply/patch:173: new blank line at EOF.
+
warning: 3 lines add whitespace errors.
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	admin_guide/diagnostics_tool.adoc
Falling back to patching base and 3-way merge...
Auto-merging admin_guide/diagnostics_tool.adoc
CONFLICT (content): Merge conflict in admin_guide/diagnostics_tool.adoc
Patch failed at 0001 edits after #8644

In response to this:

/cherrypick enterprise-3.7

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/test-infra repository.

This was referenced Jul 24, 2018
Merged
Merged
@bfallonf bfallonf deleted the diag_edits branch July 24, 2018 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kalexand-rh kalexand-rh kalexand-rh left review comments

Assignees
No one assigned
Labels
peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bfallonf @openshift-docs-preview-bot @openshift-cherrypick-robot @kalexand-rh @openshift-ci-robot