-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Typo - The example code doesn't match to the explantation. #19186
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @cainzhong! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Deploy preview for kubernetes-io-master-staging ready! Built with commit 3d9f527 https://deploy-preview-19186--kubernetes-io-master-staging.netlify.com |
/check-cla |
/assign @daminisatya |
Looks like this was wrong in commit e26aa12 (from #1196) and since then. The correction looks fine to me. @derekwaynecarr would you be willing to check that this fix is correct? It's OK to decline. |
Actually, looking again, I think the original text is correct. The example made me realize that you need to sum each set of thresholds to work out where resource pressure is first detected. |
@cainzhong I think it'd make more sense to revise the page text to clarify the summing, but leave the commands untouched. What do you think? |
/assign |
Sorry. I don't understand. What do you mean 'sum each set of thresholds to work out where resource pressure is first detected.'? |
Let's focus on the kubelet works to ensure that |
@cainzhong - whilst the original page is hard to understand, I don't think this change is correct. |
@sftim: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Do not agree with you. Let's just focus on the typo issue, do you agree it is a typo? |
/Reopen |
@cainzhong: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi @cainzhong I don't see any typo on the existing page. Did the maths at #19186 (comment) make sense to you? |
hi @sftim |
No description provided.