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

Tweak wording for Workloads overview #24602

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Oct 16, 2020

Follows on from PR #23737

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 16, 2020
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Oct 16, 2020
@sftim sftim marked this pull request as ready for review October 16, 2020 13:58
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2020
@netlify
Copy link

netlify bot commented Oct 16, 2020

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

Built with commit aa27de5

https://deploy-preview-24602--kubernetes-io-master-staging.netlify.app

These resources configure {{< glossary_tooltip term_id="controller" text="controllers" >}}
that make sure the right number of the right kind of Pod are running, to match the state
that make sure the right number of the right kind of `Pod` are running, to match the state
Copy link
Contributor

Choose a reason for hiding this comment

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

@sftim , Can you explain the updates to the resource names? I'd like to understand if this is a change to the style guide?

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 style guide was updated: 826d53c

What do you think? Does this change make the page easier to follow, or harder?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is easier to have an explicit style rule that defines something like, "always add code formatting to API resource terms and always use the API term in the content as capitalized in the code".
It is not always clear when a common term like Pod needs to use capitalization. Often a page overuses the pod term. Some of the styling of the term Pod in the page could be removed, otherwise the page starts to look like a gameboard?
Per the style guide, the other API terms use code styling. Hopefully the added styling helps readers and does not detract from reading the page.

@kbhawkey
Copy link
Contributor

@sftim
Looking at the preview, there are many formatting changes.
Also, there is a lot of information pushed into an index page.
I'd almost rather see a one sentence description (hints) about each workload (and any additional links) to get the reader to the individual page/section.
What do I think about the code formatting?
There are twenty-three resources formatted as code , several glossary tooltips, and ~sixteen intra-site links.
The page somewhat excessively uses resource terms.
There are some instances where you could have used "ingress" over Ingress (node vs Node)?
Page preview:
https://deploy-preview-24602--kubernetes-io-master-staging.netlify.app/docs/concepts/workloads/

@sftim
Copy link
Contributor Author

sftim commented Oct 20, 2020

I invite reviewers to compare:

Which is better? If it's the second one: merge this, then iterate.

@sftim sftim force-pushed the 20201015_tweak_workloads_concept_overview branch from 712b525 to ca38a6f Compare October 20, 2020 22:47
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2020
@sftim
Copy link
Contributor Author

sftim commented Oct 20, 2020

/approve cancel

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 20, 2020
@sftim sftim force-pushed the 20201015_tweak_workloads_concept_overview branch from ca38a6f to dd53659 Compare October 23, 2020 13:22
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2020
In the wider Kubernetes ecosystem, you can find third-party workload resources that provide
additional behaviors. Using a
[custom resource definition](/docs/concepts/extend-kubernetes/api-extension/custom-resources/),
you can add in a third-party workload resource if you want a specific behavior that's not part
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather link to the concept than the API details, especially whilst the API details don't hyperlink back to the concept.

Pod is running means that all the Pods on that node fail. Kubernetes treats that level
of failure as final: you would need to create a new Pod even if the node later recovers.
`Pods` have a [defined lifecycle](/docs/concepts/workloads/pods/pod-lifecycle/).
For example, once a `Pod` is running in your cluster then a critical fault on the
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use lowercase for some of the pod terms in lines 15-19?
Line 12 is clear.

additional behaviors. Using a
[custom resource definition](/docs/concepts/extend-kubernetes/api-extension/custom-resources/),
you can add in a third-party workload resource if you want a specific behavior that's not part
of Kubernetes itself. For example, if you want the ability to specify a task that runs just once
Copy link
Contributor

Choose a reason for hiding this comment

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

not part of Kubernetes itself

This is not accurate. The more common way is

not part of Kubernetes core

[custom resource definition](/docs/concepts/extend-kubernetes/api-extension/custom-resources/),
you can add in a third-party workload resource if you want a specific behavior that's not part
of Kubernetes itself. For example, if you want the ability to specify a task that runs just once
at a specific future date, neither `Job` nor `CronJob` is right for that - but you can implement or
Copy link
Contributor

Choose a reason for hiding this comment

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

No. This can be done using CronJob.

Please think of a different example.

Comment on lines 37 to 38
[`PersistentVolume`](/docs/concepts/storage/persistent-volumes/), and replicate both
the pods and their storage across different failure zones. Your code, running in the `Pods`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This introduction has gone too far from the core concept of StatefulSet. Multi-zone deployment is not limited to stateful apps. It is a different topic from stateful.

@sftim
Copy link
Contributor Author

sftim commented Nov 23, 2020

I'll move this back to Draft to mark that there's tweaks to make.

@sftim sftim marked this pull request as draft November 23, 2020 09:25
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2020
@sftim sftim force-pushed the 20201015_tweak_workloads_concept_overview branch from dd53659 to 7e3338e Compare December 8, 2020 23:35
@sftim sftim force-pushed the 20201015_tweak_workloads_concept_overview branch from 7e3338e to aa27de5 Compare December 8, 2020 23:46
@sftim
Copy link
Contributor Author

sftim commented Dec 8, 2020

I'd like another review on this (not urgent).

@sftim sftim marked this pull request as ready for review December 8, 2020 23:46
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2020
Copy link
Member

@onlydole onlydole left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you @sftim!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: onlydole

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 Dec 18, 2020
`Deployment` is a good fit for managing a stateless application workload on your cluster,
where any `Pod` in the `Deployment` is interchangeable and can be replaced if needed.
* [`StatefulSet`](/docs/concepts/workloads/controllers/statefulset/) lets you
run one or more related Pods that do track state somehow. For example, if your workload
Copy link
Contributor

@kbhawkey kbhawkey Dec 18, 2020

Choose a reason for hiding this comment

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

nit: rephrase/replace: that do track state somehow. The use of "somehow" makes me think twice.
StatefulSet workload resources let you manage stateful applications.

@kbhawkey
Copy link
Contributor

@sftim. I think the updates look good. There are a few places where "Pod" and "pod" have been switched up.
I like the additional content, though, would push further details to the actual content pages.
@tengqm , What do you think about the latest changes?
/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 18, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2020
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6bced388dcbe1b24ca34b0dd38e10bb71e77650e

@tengqm
Copy link
Contributor

tengqm commented Dec 19, 2020

/lgtm

@kbhawkey
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 19, 2020
@k8s-ci-robot k8s-ci-robot merged commit 9b35120 into kubernetes:master Dec 19, 2020
@sftim sftim deleted the 20201015_tweak_workloads_concept_overview branch September 12, 2021 10:50
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.

5 participants