-
Notifications
You must be signed in to change notification settings - Fork 14.7k
Divided headings: labels, annotations, taints #29685
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
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 @Guneetconvent2002! |
signed the CLA |
/check-cla |
Hi @Guneetconvent2002 and welcome 👋 It seems your CLA still isn't detected. Can you check that the email address used for these commits ( |
also I'm going to /assign and suggest some revisions |
That's strange cause I have made a CLA profile and it uses the same email ID as on my github |
do you mean that you're going to take over the issue or give me some suggestions to make changes?? |
Yes, ill review and suggest changes. I'll also take a look at the CLA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I requested some changes. Overall, I think it's helpful to add a link here.
You can submit a support request regarding the CLA here: https://jira.linuxfoundation.org/plugins/servlet/theme/portal/4/create/88
/assign |
👷 Deploy Preview for kubernetes-io-vnext-staging processing. 🔨 Explore the source changes: 4a7a977 🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/61a7fb2cee2e8a0008bf5e4e |
@jlbutler could maybe have a look on this and tell me what changes to make? |
Hi there! 👋 For your commits, avoid using @'s (as well as # references) in the commit message. As you're working through your edits, you may consider rebasing and collapsing into a single commit (though not necessary right now). For the content, @gcilne can you spin back through and review edits so far? Thanks everyone! |
Okay, I'll keep in mind in the future. I don't think that I need to undo that commit now. do I? |
|
||
The Seccomp annotation has been deprecated since Kubernetes v1.19 and will become non-functional in v1.25. | ||
`seccomp` is now a class inside of the pod or container. This can be changed by using `type` of the `seccompProfile` of the `securityContext`. For additinal info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revise this sentence based on this section of the style guide?
Just two small changes suggested 👍 |
I have implemented the changes that you asked for @geoffcline |
LGTM label has been added. Git tree hash: 13dcc8f903cc9e9d0e43729d05aff410cd253545
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Guneetconvent2002
Thanks for these changes @Guneetconvent2002
/lgtm
I think these are technically correct and (barely) OK to merge.
The proposed text links to https://k8s.io//docs/reference/generated/kubernetes-api/v1.22/#podsecuritycontext-v1-core
which:
- will go stale when Kubernetes v1.23 is released
- is a link to our old API reference, not the preferred current one
@Guneetconvent2002 If you want to, I think we could improve this documentation further and I have made some suggestions based on how I'd write these.
Hi @Guneetconvent2002 Could you squash this down to 1 commit? (If you're not sure how, you can instead let us know that you agree and a maintainer can do the squash for you). |
I don't really know how to squash commits together so, if a maintainer could do it, that would be great. |
/label tide/merge-method-squash |
ok, now it will be automatically squashed when approved :) |
LGTM label has been added. Git tree hash: 9ccc727cd56f6962de01118ae4e351760fdc5c48
|
@Guneetconvent2002 I'd pinged on this earlier, still unsure if this PR needs to come into dev-1.23 or main. If there isn't anything specific to the 1.23 release (or later), this PR can point at main. |
When I started working on this, the query mentioned that changes were required in dev-1.23 specifically. Since the topics where I've made changes; I was not able to find them in the main branch when I was trying to do so |
@Guneetconvent2002 can you squash your commits. One of your commit messages uses an invalid character |
I'm sorry but I don't really know how to do it. Perhaps if the admins could do it. @jlbutler could you maybe help me with that? |
@Guneetconvent2002 i'll ping on slack again and see if we can work through it there. |
I'll squash these down for you @Guneetconvent2002 using my maintainer access to write to your fork |
Co-authored-by: Qiming Teng <[email protected]> Co-authored-by: Tim Bannister <[email protected]>
|
/approve |
[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 |
/remove-label tide/merge-method-squash |
Made two separate headings as asked for in the issue #29670 and wrote some additional text under each heading explaining