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

Clean up and implement style guidelines for “Declare Network Policy” #18980

Merged
merged 1 commit into from
Feb 12, 2020
Merged

Clean up and implement style guidelines for “Declare Network Policy” #18980

merged 1 commit into from
Feb 12, 2020

Conversation

sharjeelaziz
Copy link
Contributor

  • Reworded paragraphs to reduce ambiguity.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Feb 4, 2020
@netlify
Copy link

netlify bot commented Feb 4, 2020

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

Built with commit c60ddea

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

@sharjeelaziz
Copy link
Contributor Author

/assign @zparnold

Copy link
Contributor

@danwinship danwinship left a comment

Choose a reason for hiding this comment

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

This is mostly just style fixes and I don't know what the style guide says, but the one addition of technical content is correct.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Hi @sharjeelaziz

Here's some feedback. I recommend making a few tweaks.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 6, 2020
@sftim
Copy link
Contributor

sftim commented Feb 6, 2020

I hope it's OK to clarify the PR title:
/retitle Clean up and implement style guidelines for “Declare Network Policy”

@k8s-ci-robot k8s-ci-robot changed the title Cleanup and implement style guidelines Clean up and implement style guidelines for “Declare Network Policy” Feb 6, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

My opinion is that this is almost /lgtm. The main thing that I think needs change to make this mergeable is the text blocks that read:

If you don't see a command prompt, try pressing enter.

In some ways, this isn't sample output, and it's a detail that I think is OK to skip. If keeping it in, I would prefix the sample with something like “kubectl displays a hint:”

As this is a cleanup PR, it's not making things worse but it is missing a few things I would prefer to see fixed. Everything below this line is optional to fix (in my opinion) / can wait for a future PR:

  1. min-kubernetes-server-version. @sharjeelaziz, have a look at some of the other task pages and you'll see examples of where this is set. Adding that metadata makes the {{< version-check >}} shortcode much more relevant.
  2. “And expose it via a service.”—I'd change this to “And expose it via a Service:”
  3. If I were updating the page I think I would convert the access-nginx NetworkPolicy into a downloadable resource using {{< codenew >}}

@sftim
Copy link
Contributor

sftim commented Feb 10, 2020

From a quick skim, this looks OK to me.
(not adding /lgtm because that requires a bit more of a careful look)

* Reworded paragraphs to reduce ambiguity.
* Added min-kubernetes-server-version metadata.
* Converted yaml to a downloadable resource.
@kbhawkey
Copy link
Contributor

Thanks @sharjeelaziz . This looks good.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2020
@kbhawkey
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey

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 Feb 12, 2020
@k8s-ci-robot k8s-ci-robot merged commit c8eb912 into kubernetes:master Feb 12, 2020
@sharjeelaziz sharjeelaziz deleted the patch-3 branch February 12, 2020 20:38
zacharysarah pushed a commit to zacharysarah/website that referenced this pull request Feb 21, 2020
* Reworded paragraphs to reduce ambiguity.
* Added min-kubernetes-server-version metadata.
* Converted yaml to a downloadable resource.
wawa0210 pushed a commit to wawa0210/website that referenced this pull request Mar 2, 2020
* Reworded paragraphs to reduce ambiguity.
* Added min-kubernetes-server-version metadata.
* Converted yaml to a downloadable resource.
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. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants