-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Clarify init container concept #47946
Clarify init container concept #47946
Conversation
|
Welcome @vishnuswmech! |
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thank you for the PR.
Here's some feedback
- avoid implying that manifests have to be YAML; other formats can work too (notably JSON)
- Use the active voice
- you don't need to explain too much about how Deployments work; signposting to https://kubernetes.io/docs/concepts/workloads/controllers/ may be enough
(bear in mind that Deployment is only one of several workload management APIs in Kubernetes)
We shouldn't merge this as-is, but I hope we can help you tweak it so it's good to go in.
Thanks @sftim for the suggestions. Sure, Will work on it. |
5729147
to
b1a1a97
Compare
@sftim I have made the changes based on your suggestions. Kindly review this and let me know if any further corrections needed. |
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.
/lgtm
I think there's room to improve, but I want to be clear: further tweaks are optional.
@@ -291,10 +291,9 @@ is in the `Pending` state but should have a condition `Initialized` set to false | |||
If the Pod [restarts](#pod-restart-reasons), or is restarted, all init containers | |||
must execute again. | |||
|
|||
Changes to the init container spec are limited to the container image field. | |||
Altering an init container image field is equivalent to restarting the Pod. | |||
Changes to the init container spec are limited to the container image field. Altering the image field of the init container does not restart or recreate the Pod. |
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.
Try this:
Changes to the init container spec are limited to the container image field. Altering the image field of the init container does not restart or recreate the Pod. | |
Changes to the init container spec are limited to the container image field. | |
Directly altering the `image` field of an init container does _not_ restart the | |
Pod or trigger its recreation. If the Pod has yet to start, that change may | |
have an effect on how the Pod boots up. | |
For a [pod template](/docs/concepts/workloads/pods/#pod-templates) | |
you can typically change any field for an init container; the impact of making | |
that change depends on where the pod template is used. |
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.
It is done.
|
||
Because init containers can be restarted, retried, or re-executed, init container | ||
However, since init containers can be restarted, retried, or re-executed, init container |
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.
The original text was good here I think:
However, since init containers can be restarted, retried, or re-executed, init container | |
Because init containers can be restarted, retried, or re-executed, init container |
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.
Done
|
||
Because init containers can be restarted, retried, or re-executed, init container | ||
However, since init containers can be restarted, retried, or re-executed, init container | ||
code should be idempotent. In particular, code that writes to files on `EmptyDirs` |
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.
(optionally)
code should be idempotent. In particular, code that writes to files on `EmptyDirs` | |
code should be idempotent. In particular, code that writes into any `emptyDir` volume |
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.
Make sense. It is done.
LGTM label has been added. Git tree hash: 0fb00a4522491e0b3a7b6e0b37ff13633cde1b6a
|
@vishnuswmech , what do you think of sftim's suggestions |
Thanks @sftim , now got some idea. @reylejano I will make the necessary changes and will commit it shortly. |
b1a1a97
to
80ae398
Compare
80ae398
to
27779ce
Compare
@sftim @reylejano Made the required changes based on your suggestions. Kindly check now and let me know if any further modifications needed. |
@sftim Kindly approve this. |
All suggestions are seems incorporated and preview looks good to me. |
LGTM label has been added. Git tree hash: 0e0b7bfc2d6569d882e71a68460d6ad9127d9c53
|
Thanks @T-Lakshmi for your review. |
@sftim Kindly approve this PR. |
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.
Thanks
/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 |
/retitle Clarify init container concept |
I have made the Init container concepts from both Pod and Deployment perspective.
and This is my first contribution and let me know if any changes needs to be done on PR.
Issue
Fixes: #38271