karmada-io
pkg: enable `karmada-data` flag in `karmadactl register` cmd
Description
pkg: enable karmada-data flag in karmadactl register cmd.
What type of PR is this?
/kind feature
Which issue(s) this PR fixes:
Motivated by @zhzhuang-zju's comment: https://github.com/karmada-io/karmada/pull/5334#issuecomment-2295630084
Does this PR introduce a user-facing change?:
NONE
:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.
Codecov Report
Attention: Patch coverage is 9.37500% with 29 lines in your changes missing coverage. Please review.
Project coverage is 31.12%. Comparing base (
80037ce) to head (416f6a2). Report is 35 commits behind head on master.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@
## master #5429 +/- ##
==========================================
+ Coverage 30.94% 31.12% +0.18%
==========================================
Files 638 641 +3
Lines 44307 44438 +131
==========================================
+ Hits 13712 13833 +121
+ Misses 29604 29598 -6
- Partials 991 1007 +16
| Flag | Coverage Δ | |
|---|---|---|
| unittests | 31.12% <9.37%> (+0.18%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@mohamedawnallah Places where KarmadaDir is used below also need to be modified.
https://github.com/karmada-io/karmada/blob/295dee80dc8f9cf21b152048dfc7c4660cd4f6a3/pkg/karmadactl/register/register.go#L579
https://github.com/karmada-io/karmada/blob/295dee80dc8f9cf21b152048dfc7c4660cd4f6a3/pkg/karmadactl/register/register.go#L415-L418
CACertPath also need to modified to o.CACertPath?
https://github.com/karmada-io/karmada/blob/295dee80dc8f9cf21b152048dfc7c4660cd4f6a3/pkg/karmadactl/register/register.go#L415-L418
well done~ you can add a release-note, like
`karmadactl`: enable `karmada-data` flag in command `register` for storing karmada configuration files.
/lgtm ask @chaunceyjiang for another look
@zhzhuang-zju Sounds good! However, I’ve noticed a potential improvement we could make to the initializeDirectory function in both the init and register commands. This change would allow us to handle the case where the path is a file https://github.com/karmada-io/karmada/pull/5429#discussion_r1735479866. I'll post a patch in a few minutes and would appreciate any feedback! That said, I'm totally okay with reverting back if this change doesn't meet the objective of this PR.
[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 chaunceyjiang for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Hi @zhzhuang-zju,
I apologize for the delay—this PR has been stale for a while. I will address your feedback and update the PR by the end of this week.
Thanks! 🙏
Hi @zhzhuang-zju,
I apologize for the delay—this PR has been stale for a while. I will address your feedback and update the PR by the end of this week.
Thanks! 🙏
No problem, take your time
@mohamedawnallah
I have been reviewing the karmadactl register process recently and noticed that the files under the KarmadaDir directory don't actually need to be stored locally. I'm considering whether it would be more reasonable to remove the unnecessary file localization steps instead of adding a new flag.
@mohamedawnallah I have been reviewing the karmadactl register process recently and noticed that the files under the KarmadaDir directory don't actually need to be stored locally. I'm considering whether it would be more reasonable to remove the unnecessary file localization steps instead of adding a new flag.
@zhzhuang-zju Good observation! So we close this PR for now?
@zhzhuang-zju Good observation! So we close this PR for now?
If you're willing, you can also modify the content of this PR to remove the unnecessary file localization steps.
Hi @zhzhuang-zju! I'd love to learn more and work on this, but I'm currently focused on my Karmada testing workload and schoolwork. I hope to revisit this later, but for now, I'll close it. Thanks! 🙏
Hi @zhzhuang-zju! I'd love to learn more and work on this, but I'm currently focused on my Karmada testing workload and schoolwork. I hope to revisit this later, but for now, I'll close it. Thanks! 🙏
I'll track this on issue #5477, and if you have any questions or progress, please let me know
@mohamedawnallah Hi, I would like to ask if you have already started working on this
@mohamedawnallah Hi, I would like to ask if you have already started working on this
Hi @zhzhuang-zju, Not yet. I’ll likely start on it after the LFX mentorship concludes. In the meantime, feel free to work on it if you’d like! 😊
Hi @zhzhuang-zju, Not yet. I’ll likely start on it after the LFX mentorship concludes. In the meantime, feel free to work on it if you’d like! 😊
thanks for the reply, since the karmada v1.12.0 is about to be published, and I hope this work can be contained in the new release, so I'll work on it and welcome to review~