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

updated service page #43548

Closed

Conversation

pranav-pandey0804
Copy link
Contributor

Clarification and Enhancement of Documentation Statement.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 18, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tengqm for approval. For more information see the Kubernetes Code Review Process.

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 k8s-ci-robot added the language/en Issues or PRs related to English language label Oct 18, 2023
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Oct 18, 2023
@pranav-pandey0804
Copy link
Contributor Author

fixed issue #43410

@netlify
Copy link

netlify bot commented Oct 18, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit a15edee
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6538d65541e52300084e1768
😎 Deploy Preview https://deploy-preview-43548--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.

Copy link
Member

@aj11anuj aj11anuj left a comment

Choose a reason for hiding this comment

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

.

@@ -34,7 +34,8 @@ expect that an individual Pod is reliable and durable).

Each Pod gets its own IP address (Kubernetes expects network plugins to ensure this).
For a given Deployment in your cluster, the set of Pods running in one moment in
time could be different from the set of Pods running that application a moment later.
time could be different from the set of Pods running that application a moment later
due to scaling, resources or demand.
Copy link
Member

Choose a reason for hiding this comment

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

I think Line 38 is optional change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @aj11anuj
Thanks for the review. I included

due to scaling, resources, or demand.

to clarify potential reasons behind the behavior for improved user understanding. If you believe it's optional, I'm open to your suggestion and can adjust if needed.

Copy link
Member

@Gauravpadam Gauravpadam left a comment

Choose a reason for hiding this comment

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

Thanks @pranav-pandey0804,

I think it's a good decision to make this statement less ambiguous,
However, This doesn't help much the way it is right now imo.

I would look more into pods and pod lifecycles and then make a concise statement

If you need resources, Please ask and I'll be happy to assist

@@ -34,7 +34,8 @@ expect that an individual Pod is reliable and durable).

Each Pod gets its own IP address (Kubernetes expects network plugins to ensure this).
For a given Deployment in your cluster, the set of Pods running in one moment in
time could be different from the set of Pods running that application a moment later.
time could be different from the set of Pods running that application a moment later
due to scaling, resources or demand.
Copy link
Member

Choose a reason for hiding this comment

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

Just adding these factors won't really reduce ambiguity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @Gauravpadam ,
Thank you for your feedback and your offer to assist.

I agree with your feedback that Just adding these factors won't make a huge difference in clarity.
I added these factors just to guide the readers so that they could explore the factors in detail, as the resources can be easily found within the Kubernetes documentation.

However, regarding this

This doesn't help much the way it is right now imo.

I intentionally didn't include more info to maintain a clear and focused message.
Providing extensive details might shift the text's primary focus away (from the dynamic nature of Kubernetes and its challenges in maintaining connectivity), potentially making it less effective in conveying the main point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gauravpadam
However, your suggestion is highly valued and I am eager to look at your suggestions to replace these factors in a clearer way which is also concise enough to not shift the focus of the reader from the primary message of the documentation text.
Thanks again for taking the time to look into this.

Copy link
Member

Choose a reason for hiding this comment

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 25, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 25, 2023
@pranav-pandey0804
Copy link
Contributor Author

pinging @mehabhalodiya @bradtopol ,
Hello, please take a look at this PR !

@sftim
Copy link
Contributor

sftim commented Nov 21, 2023

@pranav-pandey0804 would you be willing to revise this in light of https://github.com/kubernetes/website/pull/43548/files#r1365698319?

@pranav-pandey0804
Copy link
Contributor Author

pranav-pandey0804 commented Nov 30, 2023

hi @sftim @Gauravpadam

Thank you for your suggestion. I have carefully reviewed the feedback and incorporated the changes accordingly.
Please take a look at this revised text -

"For a given Deployment in your cluster, the set of Pods running at a specific moment may have different identities (Pod IDs and IPs) due to the dynamic nature of Kubernetes. While the number of Pods remains constant, their identities can change over time. This change is related to their identities and is not arbitrary. The dynamic nature ensures that Pods are replaced with new, near-identical instances, maintaining continuity but with different UIDs."

@pranav-pandey0804
Copy link
Contributor Author

@sftim @Gauravpadam The intention is to maintain clarity and precision in conveying the information to our users. I believe the adjustments address the concerns raised during the review process.

@sftim
Copy link
Contributor

sftim commented Nov 30, 2023

Compared to #43548 (comment), I personally prefer the text that's already in the page.

However, if another contributor feels differently, I think that's fine. Reviewers shouldn't pay any great weight to my opinion here.

@pranav-pandey0804
Copy link
Contributor Author

@sftim The current text on the page is perfectly fine but it does require the reader to know the full context, however if the readers are beginners, which they generally are, and trying to learn through documentation then they might get confused that Pods may randomly change without an explicit reason, so I just wanted to add a little bit of clarity by adding just due to scaling, resources or demand.

@pranav-pandey0804
Copy link
Contributor Author

@sftim talking about this #43548 (comment), I too find that a bit out of place with respect to the whole text.

@Gauravpadam
Copy link
Member

Gauravpadam commented Nov 30, 2023

@pranav-pandey0804 Even better, We can keep the text as is and add a reference to the pods lifecycle page for the context of readers

@pranav-pandey0804
Copy link
Contributor Author

@bradtopol @mehabhalodiya
Please take a look !

@pranav-pandey0804
Copy link
Contributor Author

Even better, We can keep the text as is and add a reference to the pods lifecycle page for the context of readers

does the original commit creates confusion or increases ambiguity?
If not then I think we can go with it !

@kbhawkey
Copy link
Contributor

Hello @pranav-pandey0804 .
The changes are minor.
Could you squash your changes into one commit?
Thanks

@AmarNathChary
Copy link
Contributor

/label tide/merge method-squash

@AmarNathChary
Copy link
Contributor

/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 Feb 1, 2024
@nate-double-u
Copy link
Contributor

I'm going to close this as stale. Thanks for proposing this solution @pranav-pandey0804, but there doesn't seem to be agreement that this change solves the issue. Feel free to re-open if you disagree, or if you have another path you'd like to try.

/close

@k8s-ci-robot
Copy link
Contributor

@nate-double-u: Closed this PR.

In response to this:

I'm going to close this as stale. Thanks for proposing this solution @pranav-pandey0804, but there doesn't seem to be agreement that this change solves the issue. Feel free to re-open if you disagree, or if you have another path you'd like to try.

/close

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.

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 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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants