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

Add description about custom profile #48154

Merged
merged 5 commits into from
Nov 17, 2024
Merged

Add description about custom profile #48154

merged 5 commits into from
Nov 17, 2024

Conversation

mochizuki875
Copy link
Member

This PR add description about custom profile.

ref:
#4292
KEP-4292

@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 1, 2024
@mochizuki875
Copy link
Member Author

/cc @ardaguclu

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 1, 2024
@netlify Netlify
Copy link

netlify bot commented Oct 1, 2024 •
edited
Loading

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 09df98e
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/67063653428b460008f1a0a0
😎 Deploy Preview https://deploy-preview-48154--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.

ardaguclu
ardaguclu reviewed Oct 1, 2024
View reviewed changes
Copy link
Member

@ardaguclu ardaguclu left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this. Overall looks good to me, just dropped a minor comment.

content/en/docs/tasks/debug/debug-application/debug-running-pod.md Outdated Show resolved Hide resolved
sftim
sftim reviewed Oct 1, 2024
View reviewed changes
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 for the PR - this is a good detail to document.

Please consider these tweaks, which will help you align to our style guide.

content/en/docs/tasks/debug/debug-application/debug-running-pod.md Outdated
@@ -718,3 +718,60 @@ Clean up the Pod when you're finished with it:
```shell
kubectl delete pod myapp
```

### Custom Profile {#custom-profile}
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
### Custom Profile {#custom-profile}
## Custom profiles for ephemeral containers {#custom-profile}

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as Debugging Profiles above, custom profile can be applied not only to an ephemeral containers, but also to a copied Pods or a debugging pods to debug nodes.
Although the following case uses an ephemeral container as an example, I think the title should remain more general, as it is now.
(While other examples can be provided, it would be redundant)

Furthermore, I think custom profile is an extension of debugging profile.
So it would be better for the Custom Profile section under Debugging Profiles to be at level ### instead of ##, or to add a new comprehensive section that consolidates the Debug Profile and Custom Profile.

content/en/docs/tasks/debug/debug-application/debug-running-pod.md Outdated Show resolved Hide resolved
content/en/docs/tasks/debug/debug-application/debug-running-pod.md Outdated Show resolved Hide resolved
content/en/docs/tasks/debug/debug-application/debug-running-pod.md Outdated Show resolved Hide resolved
content/en/docs/tasks/debug/debug-application/debug-running-pod.md Outdated Show resolved Hide resolved
content/en/docs/tasks/debug/debug-application/debug-running-pod.md Outdated Show resolved Hide resolved
@sftim
Copy link
Contributor

sftim commented Oct 1, 2024

/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Oct 1, 2024
@mochizuki875
Copy link
Member Author

@sftim
Thank your for your some comment and advice.
I've addressed and left comments.

@mochizuki875
Copy link
Member Author

@tengqm
Thank you for your comments, and I've addressed them.

tengqm
tengqm reviewed Oct 8, 2024
View reviewed changes
content/en/docs/tasks/debug/debug-application/debug-running-pod.md Outdated
Comment on lines 732 to 734
It does not support the modification of the Pod spec.
Modifications for `command`, `image`, `lifecycle`, `name` and `volumeDevices` fields
via a custom profile are not allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph doesn't make sense.
If modifications to a Pod spec is not supported, why are we talking about the specific fields then?

@tengqm
Copy link
Contributor

tengqm commented Oct 8, 2024

@mochizuki875 Please check other task pages for reference on how we structure a task pages, for example, content/en/docs/tasks/configure-pod-container/assign-cpu-resource.md.

here are some more specific guidance on this.

@mochizuki875
Copy link
Member Author

@tengqm
Thank you for your advice and providing the reference.
I understand that I'm writing part of the steps section(although it's not originally explicitly stated as a comment in the markdown), but after referring to this, I was unable to understand the decisive difference from a structural perspective, sorry...

The only point that comes to mind is the following, but could you let me know specifically what the issue is?

  • Should the section title be in the imperative rather than a noun?(e.g. Apply custom profile)
  • Should headings be ## rather than ###?
  • {{< feature-state for_k8s_version="v1.31" state="beta" >}} should not be included?

@tengqm
Copy link
Contributor

tengqm commented Oct 8, 2024

The only point that comes to mind is the following, but could you let me know specifically what the issue is?

My suggestion was to improve the structure of the page so that it is more like a "task" page.

* Should the section title be in the imperative rather than a noun?(e.g. Apply custom profile)

Yes.

* Should headings be `##` rather than `###`?

## stands for <H2> in HTML, ### stands for <H3> in HTML. The top-level headings for a page should be ##.

* `{{< feature-state for_k8s_version="v1.31" state="beta" >}}` should not be included?

The feature-state stanza is a Docsy shortcode. It is included to indicate that the content
is about a feature under development. Use it when necessary.

@mochizuki875
Copy link
Member Author

@tengqm

My suggestion was to improve the structure of the page so that it is more like a "task" page.

Are you suggesting that, rather than limiting the scope of this PR to the custom profile, we should revamp the entire page to follow the task style in this PR?

If so, how about addressing it as a separate issue?
The purpose of this PR is to inform users about the Custom Profile, which has not yet been documented, and I believe this provides some value to the users.
e.g.) related kubernetes/kubectl#1650

@tengqm
Copy link
Contributor

tengqm commented Oct 9, 2024

@tengqm

My suggestion was to improve the structure of the page so that it is more like a "task" page.

Are you suggesting that, rather than limiting the scope of this PR to the custom profile, we should revamp the entire page to follow the task style in this PR?

If so, how about addressing it as a separate issue?

I'm okay with that, considering that this PR is not about a new task page.
But please do as much as you can to follow the desired structure of a task page.

@mochizuki875
Copy link
Member Author

@tengqm
OK, so I've restructured the section regarding profiles with some fixes.

@tengqm
Copy link
Contributor

tengqm commented Oct 9, 2024

/lgtm

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

LGTM label has been added.

Git tree hash: 7846deee2092b94c295978160cc1da48fe0d99d0

@tengqm tengqm added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 9, 2024
@mochizuki875
Copy link
Member Author

PTAL?

@ardaguclu
Copy link
Member

Thank you
/lgtm

dipesh-rawat
dipesh-rawat reviewed Nov 17, 2024
View reviewed changes
Copy link
Member

@dipesh-rawat dipesh-rawat left a comment

Choose a reason for hiding this comment

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

Thanks!

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dipesh-rawat

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 Nov 17, 2024
@k8s-ci-robot k8s-ci-robot merged commit 958ed3b into kubernetes:main Nov 17, 2024
6 checks passed
@windsonsea windsonsea mentioned this pull request Nov 17, 2024
Merged
This was referenced Nov 18, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@dipesh-rawat dipesh-rawat dipesh-rawat left review comments

@ardaguclu ardaguclu ardaguclu left review comments

@tengqm tengqm tengqm left review comments

@kbhawkey kbhawkey Awaiting requested review from kbhawkey

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@tengqm tengqm

@ardaguclu ardaguclu

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/cli Categorizes an issue or PR as relevant to SIG CLI. 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
SIG CLI
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mochizuki875 @sftim @tengqm @k8s-ci-robot @ardaguclu @dipesh-rawat