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.

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

Move node shutdown section to cluster-administration #45996

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Move node shutdown section to cluster-administration #45996

merged 1 commit into from
Apr 29, 2024

Conversation

mrgiles
Copy link
Contributor

@mrgiles mrgiles commented Apr 24, 2024

Move graceful/non-graceful node shutdown section from concepts/architecture/nodes.md to concepts/cluster-administration/nodes.md. Fixes #45690

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

netlify bot commented Apr 24, 2024 •
edited
Loading

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit f32bcaf
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/662a88115f4057000824d0f3
😎 Deploy Preview https://deploy-preview-45996--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mrgiles
Copy link
Contributor Author

mrgiles commented Apr 24, 2024

FYI @reylejano

sftim
sftim reviewed Apr 24, 2024
View reviewed changes
content/en/docs/concepts/cluster-administration/nodes.md Outdated
Comment on lines 2 to 4
reviewers:
- reylejano
title: Nodes
Copy link
Contributor

@sftim sftim Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Try this:

Suggested change
reviewers:
- reylejano
title: Nodes
title: Node Shutdowns

Copy link
Contributor

Choose a reason for hiding this comment

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

(You could change the filename to match)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing.
Do you mean change nodes.md to nodes_shutdown.md? What if somebody wants to add more node-related content under the same section, do they have to create a new nodes_foo.md file?

Copy link
Contributor

Choose a reason for hiding this comment

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

node-shutdown.md with a -, but yes. Having another page in the section about nodes is very likely; we already have one, and there are plans to rename it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

@mrgiles mrgiles force-pushed the 45690_move_node_shutdown_to_cluster_admin branch from a556c1f to 6887bd4 Compare April 24, 2024 17:48
@mrgiles mrgiles requested a review from sftim April 24, 2024 18:06
@mrgiles mrgiles force-pushed the 45690_move_node_shutdown_to_cluster_admin branch from 6887bd4 to 372322b Compare April 24, 2024 21:40
dipesh-rawat
dipesh-rawat reviewed Apr 25, 2024
View reviewed changes
content/en/docs/concepts/cluster-administration/node-shutdown.md Outdated
Comment on lines 3 to 4
api_metadata:
- kind: "Node"
Copy link
Member

Choose a reason for hiding this comment

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

The api-metadata section needs a apiVersion field to be complete. Let's either add it in (preferred option) or consider removing the api-metadata section altogether from this new page.

Below is a suggestion on how it would look with the addition.

Suggested change
api_metadata:
- kind: "Node"
api_metadata:
- apiVersion: "v1"
kind: "Node"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

@mrgiles mrgiles force-pushed the 45690_move_node_shutdown_to_cluster_admin branch from 372322b to 5a1c768 Compare April 25, 2024 00:15
@mrgiles mrgiles requested a review from dipesh-rawat April 25, 2024 00:20
@mrgiles mrgiles force-pushed the 45690_move_node_shutdown_to_cluster_admin branch from 5a1c768 to 5d06286 Compare April 25, 2024 00:43
@mrgiles
Move node shutdown section to cluster-administration
f32bcaf 
Update doc title

Rename file/update link

Update api_metadata

Update what's next

Remove api_metadata, add tootip
@mrgiles mrgiles force-pushed the 45690_move_node_shutdown_to_cluster_admin branch from 5d06286 to f32bcaf Compare April 25, 2024 16:42
@mrgiles mrgiles requested a review from sftim April 25, 2024 16:46
sftim
sftim reviewed Apr 25, 2024
View reviewed changes
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.

Thanks

/lgtm

content/en/docs/concepts/cluster-administration/node-shutdown.md
---
title: Node Shutdowns
content_type: concept
weight: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this, but the number isn't used right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove the weight line?

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

LGTM label has been added.

Git tree hash: 19060840abfd9c6a1c105e62207ddd09dbd950dc

@mrgiles mrgiles requested a review from sftim April 25, 2024 18:13
@reylejano
Copy link
Member

Thank you
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano

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 Apr 29, 2024
@k8s-ci-robot k8s-ci-robot merged commit b293d1a into kubernetes:main Apr 29, 2024
6 checks passed
@windsonsea windsonsea mentioned this pull request Apr 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@mickeyboxell mickeyboxell Awaiting requested review from mickeyboxell

@dipesh-rawat dipesh-rawat Awaiting requested review from dipesh-rawat

@sftim sftim Awaiting requested review from sftim

Assignees

@sftim sftim

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move non-graceful node shutdown section from concepts/architecture/nodes.md to concepts/cluster-administration
5 participants
@mrgiles @k8s-ci-robot @reylejano @dipesh-rawat @sftim