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

Add description in qos-pod-4.yaml #25904

Closed
wants to merge 1 commit into from
Closed

Conversation

ydFu
Copy link
Member

@ydFu ydFu commented Jan 1, 2021

Add description that distinguish between container-1 and container-2 make it easy to read.

Signed-off-by: ydFu [email protected]

@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 1, 2021
@k8s-ci-robot k8s-ci-robot added sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 1, 2021
@netlify
Copy link

netlify bot commented Jan 1, 2021

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

🔨 Explore the source changes: e6799c5

🔍 Inspect the deploy logs: https://app.netlify.com/sites/kubernetes-io-master-staging/deploys/5fef100247a2a9000ade5a97

😎 Browse the preview: https://deploy-preview-25904--kubernetes-io-master-staging.netlify.app

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 2, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d4396ef200ed5d7c9dda347b9e11a8b97bfe0c38

@ydFu
Copy link
Member Author

ydFu commented Jan 4, 2021

@sftim
Thank for review this PR.

@ydFu
Copy link
Member Author

ydFu commented Jan 7, 2021

/assign @zparnold
Thanks.

@@ -5,12 +5,12 @@ metadata:
namespace: qos-example
spec:
containers:

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the added comments make sense. Maybe an existing member can comment for cofirmation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR already has an LGTM. With another LGTM I can approve it; I would like a second review from a Kubernetes member before this merges.

Copy link
Member Author

@ydFu ydFu Jan 28, 2021

Choose a reason for hiding this comment

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

/assign @sftim
l thinked this PR be reviewed from other Kubernetes member(@Aut0R3V),
Thanks for your help.

Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure if we need a comment about the limit provided to the memory since it’s already defined on the manifest though.

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise it’s lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Aut0R3V
To complete the pull request process, please assign sftim after the PR has been reviewed.
You can assign the PR to them by writing /assign @sftim in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

@ydFu: GitHub didn't allow me to request PR reviews from the following users: Aut0R3V.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/assign @sftim
l thinked this PR be reviewed from other Kubernetes member(@Aut0R3V),
Could you help to add "approved label" ?
Thanks for your help.

/cc @Aut0R3V @rajeshdeshpande02 @steveperry-53
Could you help to revirew this PR ?
Thanks for your help.

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.

@ydFu
Copy link
Member Author

ydFu commented Feb 6, 2021

/assign @tengqm
Please help review this PR.
Let me know if there is anything need to be revised,and thank a lot.

@tengqm
Copy link
Contributor

tengqm commented Feb 7, 2021

@ydFu TBH, I don't think we really need comments like these. The manifest is already straightforward.

@ydFu
Copy link
Member Author

ydFu commented Feb 8, 2021

Thanks for your guidance.
At the time I thought the code in qos-pod-4.yaml of this article in configure-pod-container/quality-service-pod.md that is for
introduction to getting started,so just added description to replace the special blank line.

@tengqm
Copy link
Contributor

tengqm commented Feb 8, 2021

@ydFu All the YAML manifests under the examples directory are associated with some contexts. These examples are provided to save the users some time on manually creating every YAML file. In this case, I'd prefer leaving some room for users to digest the manifests. Comments are only necessary when something unusual or something worth noting.

@ydFu
Copy link
Member Author

ydFu commented Feb 8, 2021

@tengqm
Thank you for your instruction and advice.
It let me know the norms in the YAML manifests under the examples directory.
The next submission is similar the adjustment,I will pay special attention.

@ydFu ydFu closed this Feb 8, 2021
@ydFu ydFu deleted the add-description branch August 17, 2022 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants