Skip to content

Conversation

Aaina26
Copy link
Contributor

@Aaina26 Aaina26 commented Jan 8, 2025

Fixes: #48803

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language labels Jan 8, 2025
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 8, 2025
Copy link

netlify bot commented Jan 8, 2025

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit dca9071
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/679b4ca4a337920007e2ae47
😎 Deploy Preview https://deploy-preview-49360--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.

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. Let's make sure to document the node-role.kubernetes.io/* node label too.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 22, 2025
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.

How about these changes? They'd align better with the style guide.

However, I also think that making this change as-is makes the docs overall better, so:
/lgtm

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

LGTM label has been added.

Git tree hash: 5cfe56a8f3df5d112933da799f6e4bcac4dfe5ce

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2025
@k8s-ci-robot k8s-ci-robot requested a review from sftim January 22, 2025 09:28
@Aaina26
Copy link
Contributor Author

Aaina26 commented Jan 22, 2025

Thanks for the suggestions. I have updated the PR to better suite the style guide.

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.

One more change and this is dead right (see inline feedback)

/lgtm

If we merge this, the docs are less wrong.

@@ -140,6 +140,11 @@ When you want to create Node objects manually, set the kubelet flag `--register-
You can modify Node objects regardless of the setting of `--register-node`.
For example, you can set labels on an existing Node or mark it unschedulable.

You can set optional node role(s) for nodes by adding one or more `node-role.kubernetes.io/<role>: <role>` labels to the node where characters of `<role>`
are limited by the [syntax](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) rules for labels.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
are limited by the [syntax](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) rules for labels.
are limited by the [syntax](/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) rules for labels.

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

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

LGTM label has been added.

Git tree hash: d2da1cf1d3ff8ef5ce395ab4740a852f9a56e6f0

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2025
@k8s-ci-robot k8s-ci-robot requested a review from sftim January 26, 2025 16:23
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.

/lgtm

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

LGTM label has been added.

Git tree hash: 5733119b5ac6e58e10908f7413db2e2168c5e5d2

You can set optional node role(s) for nodes by adding one or more `node-role.kubernetes.io/<role>: <role>` labels to the node where characters of `<role>`
are limited by the [syntax](/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) rules for labels.

Kubernetes ignores the label value for node roles;by convention, you can set it to the same string you used for the node role in label key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Kubernetes ignores the label value for node roles;by convention, you can set it to the same string you used for the node role in label key.
Kubernetes ignores the label value for node roles; by convention, you can set it to the same string you used for the node role in the label key.

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 have addressed this in the new commit. Thanks for pointing this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are welcome

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2025
@k8s-ci-robot k8s-ci-robot requested a review from sftim January 30, 2025 09:55
@Aaina26
Copy link
Contributor Author

Aaina26 commented Feb 11, 2025

Hi @sftim and @network-charles..
PTAL

@network-charles
Copy link
Contributor

/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 11, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d82c6e199568f8de4135c3ccb23214fd7c2a6116

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.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

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 16, 2025
@k8s-ci-robot k8s-ci-robot merged commit f80c848 into kubernetes:main Feb 16, 2025
6 checks passed
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node-role.kubernetes.io/* docs are wrong
4 participants