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

Update debug-cluster and crictl files to get them consistent with the… #13183

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

DanyC97
Copy link
Contributor

@DanyC97 DanyC97 commented Mar 14, 2019

… style guidelines

Two tasks from #12740 were covered:

question for sig-docs members:

should we rename the task title title: "Monitor, Log and Debug" to Monitoring, Logging and Debugging to keep alignment/ consistency ?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 14, 2019
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Mar 14, 2019
@netlify
Copy link

netlify bot commented Mar 14, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 3281e0a

https://deploy-preview-13183--kubernetes-io-master-staging.netlify.com

@DanyC97
Copy link
Contributor Author

DanyC97 commented Mar 15, 2019

@cody-clark please help review, thank you

Copy link
Contributor

@cody-clark cody-clark left a comment

Choose a reason for hiding this comment

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

Everything looks good for /docs/tasks/debug-application-cluster/crictl.md, but I could use some justification for getting rid of the serial commas and the h5s

@@ -41,7 +41,7 @@ Run Jobs using parallel processing.

Configure load balancing, port forwarding, or setup firewall or DNS configurations to access applications in a cluster.

## Monitoring, Logging, and Debugging
## Monitoring, Logging and Debugging
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm reluctant to approve getting rid of a serial comma... XD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason here was purely grammer, nothing else. and i tried to align this file with content/en/docs/tasks/debug-application-cluster/_index.md.

At the same time i've also asked in the PR description if

should we rename the task title title: "Monitor, Log and Debug" to Monitoring, Logging and Debugging to keep alignment/ consistency ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Renaming the task title for consistency is a good idea.

I'm still not sold on deleting the the serial comma. "Monitoring, Logging, and Debugging" is just as correct as "Monitoring, Logging and Debugging". And I'm not ready to instigate the Oxford comma debate in sig-docs!


## A general overview of cluster failure modes

This is an incomplete list of things that could go wrong, and how to adjust your cluster setup to mitigate the problems.

Root causes:
##### Root causes:
Copy link
Contributor

Choose a reason for hiding this comment

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

An H5 seems very specific. Was there a reason to go this deep?

Copy link
Contributor Author

@DanyC97 DanyC97 Mar 15, 2019

Choose a reason for hiding this comment

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

rational was to articulate the 3 pillars: Root causes, Specific scenarios & Mitigations from the rest of the actions/ text.

Initially tried to just make the text bold but in my local preview website it didn't make the difference i was expecting hence h5.

Any alternative ?

Copy link
Contributor

@cody-clark cody-clark Mar 18, 2019

Choose a reason for hiding this comment

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

I completely agree that those pillars should be differentiated. Don't H3s work here?

I prefer semantic markup for hierarchy over appearance. There's also the accessibility consideration: https://www.w3.org/WAI/tutorials/page-structure/headings/

Skipping heading ranks can be confusing and should be avoided where possible: Make sure that a <h2> is not followed directly by an <h4>, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, will give it a go and then you will see if it makes sense with h3 as part of the review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cody-clark please help review again, made the change from h5 to h3

Copy link
Contributor

@cody-clark cody-clark left a comment

Choose a reason for hiding this comment

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

Thanks, @DanyC97! Sorry for the back and forth

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cody-clark

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2019
@k8s-ci-robot k8s-ci-robot merged commit d85ac26 into kubernetes:master Mar 19, 2019
@DanyC97
Copy link
Contributor Author

DanyC97 commented Mar 19, 2019

no problem at all @cody-clark , i'm all good

@DanyC97 DanyC97 deleted the task-dappc-dc branch March 19, 2019 17:44
claudiajkang pushed a commit that referenced this pull request Apr 2, 2019
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants