Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Migrate reference details out Service concept #36675
Migrate reference details out Service concept #36675
Changes from all commits
1d68a02
d760176
ca9e396
0912bf0
25c2586
b581bd4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
You may want to make 'kube-proxy' a link or a glossary.
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.
For this PR, I was aiming to retain the original text. Subsequent PRs could tidy that up.
If some of my other PRs merge, I can shrink #30817 until it reaches a reviewable size. That PR includes more fixes than just the reorganisation that this PR covers.
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.
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.
For this PR, I was aiming to retain the original text. Subsequent PRs could tidy that up.
If some of my other PRs merge, I can shrink #30817 until it reaches a reviewable size. That PR includes more fixes than just the reorganisation that this PR covers.
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'm not sure if the application protocol has to be bound to a LoadBalancer service. Maybe we can use it for ClusterIP and/or NodePort services as well?
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.
If you are annotating a Service, where the
type
of the Service is set toLoadBalancer
, we can assume there's a load balancer. If you're not, this section isn't relevant.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.
This section is not about LoadBalancer service, right?
We are confusing users with this revision.
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.
What are the implications to users? In other words, why should they care about these details?
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 we fix that as a follow up after the refactor part of the changes are in?
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.
IMHO, in a refactor PR, we can move the existing text without any modifications. Such a PR is a pure refactor one. For newly added contents, we may want to make sure we are not introducing defects. If a PR is already too large, we may want to consider whether it is possible to split such a PR to smaller ones. When we identify that something is more appropriate for a follow-up PR, we may want to ensure that an issue is filed before the current PR is kicked in.
Does this make senses to you?
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 agree with each of those principles @tengqm but I'm not sure if we agree on the interpretation of them.
I don't see a way to make this PR smaller and still be useful.