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

Various Server-Side Apply improvements #36293

Merged
merged 3 commits into from
Sep 30, 2022
Merged

Various Server-Side Apply improvements #36293

merged 3 commits into from
Sep 30, 2022

Conversation

apelisse
Copy link
Member

@justinsb told me he wasn't sure what permissions were needed to use server-side apply, and we're not doing a great job explaining this today so I added that sub-section about it. I think that's the right location since we also explain that ssa is a patch at that location. I also noticed a caution block that is obsolete since GA. And while I was editing this file, I just normalized every instances to Server-Side Apply.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 25, 2022
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Aug 25, 2022
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Aug 25, 2022
@apelisse
Copy link
Member Author

Ah! you've created a main branch, but the master branch is neither removed nor an alias to main, that's super confusing...

@apelisse
Copy link
Member Author

Reference: kptdev/kpt#3497 (comment)

@netlify
Copy link

netlify bot commented Aug 25, 2022 •
edited
Loading

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit cae95ce
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/63321308fac71200086a9f0b
😎 Deploy Preview https://deploy-preview-36293--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 settings.

@sftim
Copy link
Contributor

sftim commented Aug 25, 2022

If any of #31261 is useful, feel free to borrow from that PR.

@sftim
Copy link
Contributor

sftim commented Aug 25, 2022

Also, I don't think this repo has a master branch. Check https://github.com/kubernetes/website/tree/master/

@apelisse
Copy link
Member Author

Yeah, not sure why I ended-up having a master branch on my remote that doesn't match the main branch.

Also, I love your other PR, but it's fairly big and I'm afraid trying to do too much (merging the two) would significantly delay that quick fix here.

@sftim
Copy link
Contributor

sftim commented Aug 25, 2022

What I mean is, you can borrow a paragraph or so if that helps you @apelisse - not a merge of all the work so far.

No problem with you borrowing sentences or paragraphs; I wouldn't object to a Co-Authored-By: credit if you do.

@apelisse
Copy link
Member Author

Yeah, but I'm afraid that's going to take me a lot of time and possible push-back that I don't really have time to afford right now :-)

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2022
@apelisse
Copy link
Member Author

OK Fixed conflicts. @sftim, I'll see if I can take some of your PR changes into another smaller PR later if you don't mind.

chrisnegus
chrisnegus suggested changes Aug 26, 2022
View reviewed changes
Copy link
Contributor

@chrisnegus chrisnegus left a comment

Choose a reason for hiding this comment

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

Found one "Server-Side" you missed. Otherwise lgtm.

content/en/docs/reference/using-api/server-side-apply.md Outdated
@@ -465,12 +465,18 @@ kubectl apply --server-side --field-manager=my-manager [--dry-run=server]

## API Endpoint

With the Server Side Apply feature enabled, the `PATCH` endpoint accepts the
With the Server-Side Apply feature enabled, the `PATCH` endpoint accepts the
additional `application/apply-patch+yaml` content type. Users of Server Side
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
additional `application/apply-patch+yaml` content type. Users of Server Side
additional `application/apply-patch+yaml` content type. Users of Server-Side

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch, my very simple search-and-replace didn't catch this case ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I found another instance!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. That is one reason I like to start and end each sentence on its own line. One's searches are not foiled by the end of a line.

@apelisse
Copy link
Member Author

I improved the regex and even found a few more.

Sea-n
Sea-n reviewed Sep 15, 2022
View reviewed changes
Copy link
Member

@Sea-n Sea-n left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

In addition, I think the first two commits (remove & add section) should be squash into one. How do you think?

content/en/docs/reference/using-api/server-side-apply.md Outdated
Apply can send partially specified objects as YAML to this endpoint. When
applying a configuration, one should always include all the fields that they
have an opinion about.

### RBAC And permissions
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### RBAC And permissions
### RBAC and Permissions

or

Suggested change
### RBAC And permissions
### RBAC and permissions

Copy link
Contributor

Choose a reason for hiding this comment

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

@apelisse , What do you think about the second suggestion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the ping, missed that somehow, sending a fix with the second suggestion!

@kbhawkey
Copy link
Contributor

Page preview:
https://deploy-preview-36293--kubernetes-io-main-staging.netlify.app/docs/reference/using-api/server-side-apply/

@kbhawkey
Copy link
Contributor

@apelisse , Can you rebase into a single commit?
/lgtm

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

LGTM label has been added.

Git tree hash: d4619db0cca5c297d358de85414b4a05d94f4ea3

@Sea-n
Copy link
Member

Sea-n commented Sep 21, 2022

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 21, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2022
@apelisse
Copy link
Member Author

Updated

@kbhawkey
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey

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 Sep 27, 2022
@windsonsea
Copy link
Member

/lgtm

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

LGTM label has been added.

Git tree hash: 116ad6524ec5024ed7f2d47d1691041cc5ea8289

@k8s-ci-robot k8s-ci-robot merged commit 2684888 into kubernetes:main Sep 30, 2022
@windsonsea windsonsea mentioned this pull request Sep 30, 2022
Merged
yanrongshi pushed a commit to yanrongshi/website that referenced this pull request Oct 6, 2022
@apelisse @yanrongshi
Various Server-Side Apply improvements (kubernetes#36293)
b25dbc0 
* Remove obsolete caution block for ssa-ing sub-resources

* ssa: Add RBAC/Permissions section

* ssa: Make everything consistent into "Server-Side Apply"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisnegus chrisnegus chrisnegus requested changes

@Sea-n Sea-n Sea-n left review comments

@lavalamp lavalamp Awaiting requested review from lavalamp

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@kbhawkey kbhawkey Awaiting requested review from kbhawkey

Assignees

@kbhawkey kbhawkey

@windsonsea windsonsea

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@apelisse @sftim @kbhawkey @k8s-ci-robot @Sea-n @windsonsea @chrisnegus