-
Notifications
You must be signed in to change notification settings - Fork 885
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
Proposal: Add API Server Service Information to KarmadaStatus
#5599
Proposal: Add API Server Service Information to KarmadaStatus
#5599
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5599 +/- ##
==========================================
+ Coverage 34.84% 42.30% +7.46%
==========================================
Files 645 655 +10
Lines 44861 55756 +10895
==========================================
+ Hits 15633 23590 +7957
- Misses 28021 30652 +2631
- Partials 1207 1514 +307
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
docs/proposals/karmada-operator/api-server-service-status/README.md
Outdated
Show resolved
Hide resolved
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.
/assign
b994313
to
dc35f83
Compare
The services created by Karmada can have four different types. In order to make better decisions regarding the most suitable API form, I conducted tests on different service types and documented for reference: doc link I hope this will be helpful to all ! |
Thanks @chaosi-zju for your effort. Yes, we noticed that currently, karmada-operator allows people to config all kinds of Service types, and @jabellard analyzed it in the discussion above. But, I can't access the doc, just sent a request for that :) :) |
@chaosi-zju , thank you for this. I'd love to learn more about your findings. I just sent you a request to get access to the doc. |
docs/proposals/karmada-operator/api-server-service-status/README.md
Outdated
Show resolved
Hide resolved
docs/proposals/karmada-operator/api-server-service-status/README.md
Outdated
Show resolved
Hide resolved
dc35f83
to
62627ee
Compare
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.
Hi @jabellard
Generally looks good to me.
From the test report @chaosi-zju provided on above, we can tell that:
- Currently we supports 2 Service types, there are
ClusterIP
andNodePort
. - The
Loadbalancer
is not supported yet, but I think it makes sense to enhance it this time. See the use case on When installing karmada using the operator, provide a way to annotate karmadaAPIServer services #4634.
So I just opened #5767 to track all of this. Any thoughts?
docs/proposals/karmada-operator/api-server-service-status/README.md
Outdated
Show resolved
Hide resolved
@RainbowMango , thanks for sharing this and also to @chaosi-zju for generating this thorough report. I'll keep on eye on these action items. I just pushed some changes to address your comments. |
Thanks Joe, I think we are ready to go after squashing the commits. We can start to implemente it now. |
Signed-off-by: Joe Nathan Abellard <contact@jabellard.com>
f639d97
to
e8c81c5
Compare
Just squashed. I started the implementation, and will send a PR for that soon. |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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 |
The failing test is unrelated. We can retest it by leaving a comment tracked by #5136 |
/retest |
This statement seems to be misspelled, |
Right, thanks. Updated on #5599 (review). |
What type of PR is this?
/kind design
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: