-
Notifications
You must be signed in to change notification settings - Fork 919
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: Custom CRD Download Strategy Support for Karmada Operator #5145
Conversation
Welcome @jabellard! It looks like this is your first PR to karmada-io/karmada 🎉 |
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 #5145 +/- ##
==========================================
+ Coverage 28.21% 28.26% +0.04%
==========================================
Files 632 632
Lines 43568 43739 +171
==========================================
+ Hits 12294 12361 +67
- Misses 30378 30477 +99
- Partials 896 901 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7b17c7f
to
9da129a
Compare
/cc @calvin0327 PTAL. |
@jabellard Thank you very much for the excellent proposal. |
@calvin0327 Is there a chance for you to present this week's community meeting and talk with @jabellard and this proposal? |
// Omit if the intent is to provision a new Karmada instance with control plane components of the same version as the Karmada operator's release version. | ||
// Otherwise, it should be explicitly set to ensure the right version of the CRDs is installed for this instance. | ||
// +optional | ||
CRDRemoteURL string `json:"crdRemoteURL,omitempty"` |
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.
In fact, within our company, we have copied the crd file into the image of karmada-operator.
So what I want to ask is, if my crd file is stored in the image, how should I fill in the crdRemoteURL?
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.
we have copied the crd file into the image of karmada-operator.
How to do that? What I can imagine there are two ways to do that:
- Using a private karmada-operator image, the tarball can be put into the image
- Mount the tarball into the pod, by a ConfigMap when deploying the operator
The tarball is located in the cache, so karmada-opeator doesn't need to download the tarball from GitHub.
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.
So what I mean is, should crdRemoteURL
support a format like file:///var/loca/crds
?
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 is possible and can be supported, but can you please elaborate more on why that strategy would be preferred to downloading the CRDs from a remote location? How would you manually manage storage for multiple versions of the CRDs? Would that not require more work and maintenance on your part?
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.
Kindly ping @chaunceyjiang
We need your input here :)
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.
but can you please elaborate more on why that strategy would be preferred to downloading the CRDs from a remote location?
Simply put, we don't want our end users to download from remote http/https servers. Because that requires our end users to do some extra work, such as preparing additional http/https servers, etc. We would rather have the karmada-operator read the crd file from the karmada-operator image.
In this way, users only need to use our provided Web-UI
function to deploy a brand new karmada instance with one click
. There's no need for extra attention on where the crd is stored.
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 get it, you build the image on your own, like put the tarball to the cache directory, then the karmada-operator won't download it from github.
@RainbowMango , thanks so so much for the feedback. I'll push an update to the proposal based on what we discussed during the meeting. |
@RainbowMango , I just submitted a pr for the implementation of this proposal. |
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
910d946
to
25e6fda
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.
/lgtm
@chaunceyjiang Do you have any further comments?
Signed-off-by: Joe Nathan Abellard <[email protected]> Fix typos Signed-off-by: Joe Nathan Abellard <[email protected]> Update hash algo Signed-off-by: Joe Nathan Abellard <[email protected]> Address commands Signed-off-by: Joe Nathan Abellard <[email protected]> Address commands Signed-off-by: Joe Nathan Abellard <[email protected]> Address commands Signed-off-by: Joe Nathan Abellard <[email protected]> Fix typos Signed-off-by: Joe Nathan Abellard <[email protected]> Fix typos Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]>
25e6fda
to
4611c55
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.
/lgtm
Thanks~
Perfect! I opened the implementation 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.
/approve
Thanks.
[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 |
What type of PR is this?
/kind design
What this PR does / why we need it:
Details are in the design document.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: