cluster-api
cluster-api copied to clipboard
✨ feature(clusterctl): Follow XDG Directory standard for config/data/... files
What this PR does / why we need it: This PR switches the hardcoded paths ($HOME/.cluster-api
) to be XDG conform ($XDG_CONFIG_HOME/cluster-api
).
This removes clutter in the users' home directory.
Open questions:
-
I tried adding backwards compatibility to the old path inside the
reader_viper.go
but I'm not sure if this works for everything -
The script
cmd/clusterctl/hack/create-local-repository.py
creates adev-repository
at$HOME/.cluster-api/dev-repository
. I see three solutions to this;- The script uses an xdg library, which has to be installed on the users' system, which we'll mention in the docs
- The script does everything by itself, without needing dependencies
- This could be a lot of work, especially for Windows and macOS, as I can't test these systems
- The documentation will mention to overwrite
XDG_CONFIG_HOME
while using the script, this could be weird
I personally vote for the first possibility
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: cwrau / name: Chris Werner Rau (a0d0c1d17124d5b446b4b11032d2f14df15abdd5)
Welcome @cwrau!
It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @cwrau. Thanks for your PR.
I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test
label.
I understand the commands that are listed here.
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.
/ok-to-test
@cwrau: The following test failed, say /retest
to rerun all failed tests or /retest-required
to rerun all mandatory failed tests:
Test name | Commit | Details | Required | Rerun command |
---|---|---|---|---|
pull-cluster-api-apidiff-main | d911f541cca939a62bcd07b7bf50b7e1d2748c97 | link | false | /test pull-cluster-api-apidiff-main |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
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. I understand the commands that are listed here.
The apidiff is failing, but that is expected, what can I do?
The api diff isn't merge blocking if the maintainers think this is worth merging without an API revision - the check is really informing rather than blocking for API changes so don't worry about trying to pass it :)
The api diff isn't merge blocking if the maintainers think this is worth merging without an API revision - the check is really informing rather than blocking for API changes so don't worry about trying to pass it - it would be good if you could sign the CLA though 🙂
The api diff isn't merge blocking if the maintainers think this is worth merging without an API revision - the check is really informing rather than blocking for API changes so don't worry about trying to pass it - it would be good if you could sign the CLA though slightly_smiling_face
Yes, my boss is working on the CLA 👍️
CLA has been accepted 👍️
/help
Good to hear! I'm not sure what you're looking for with the help tag though :laughing:
Good to hear! I'm not sure what you're looking for with the help tag though 😆
I just wanted to add the help-wanted
label, but apparently that doesn't work this way 🤣
I think we normally only add that label to issues where we want someone to take them up - are you looking to have someone else pick up this PR? If so it might be better to create an issue describing the goals, link this draft PR and then add the help wanted tag.
Ah, ok, then I misunderstood the point of that label 😅
No, I can keep this PR 👍
@cwrau Is the PR ready for review? (just asking because we usually use Draft/WIP to ~ signal implementation is still in progress and folks should wait for reviews except if they are explicitly asked to provide a review)
cc @ykakarap
@sbueringer
Ah, I see, yeah, I still need to have the questions answered 👍
By anyone, really 😅
Got it.
cc @ykakarap I'll also take a closer look soon
prior art from kubectl
https://github.com/kubernetes/kubernetes/pulls?q=is%3Apr+XDG
[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 timothysc for approval by writing /assign @timothysc
in a comment. 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
cc @fabriziopandini
@cwrau: PR needs rebase.
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.
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/stale
is applied - After 30d of inactivity since
lifecycle/stale
was applied,lifecycle/rotten
is applied - After 30d of inactivity since
lifecycle/rotten
was applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale
- Mark this issue or PR as rotten with
/lifecycle rotten
- Close this issue or PR with
/close
- Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
/remove-lifecycle stale
@cwrau Still interested in working on this?
I'm favour this change to using the XDG standard. It would be easy to have backwards compatibility for users with the current ~/.cluster-api
@cwrau would you like to continue working on this?
I'm favour this change to using the XDG standard. It would be easy to have backwards compatibility for users with the current
~/.cluster-api
@cwrau would you like to continue working on this?
I can continue this, but it's getting quite frustrating how long this is taking 🙈
I rebased this so many times now, is there anything preventing this from being merged after the next rebase? @vincepri
@cwrau - could you change this from a draft to an open PR? I don't think this is getting reviews because draft can mean WIP - and definitely does when I see this in my notifications.
The tests that are failing are checks that verify that the path is $HOME/.cluster-api
😅
The tests that are failing are checks that verify that the path is
$HOME/.cluster-api
sweat_smile
@cwrau - you will need to update the paths in the tests if this is going to be the default from now. This means for example updating the path here, which is causing make test
to fail
/retest
/retest
@cwrau - with the open questions in the PR description, the first one is answered. How about the second question?
this generally looks ok to me, but i have a concern about breaking our osx and windows users, also a nit inline, and do we need to update main/cmd/clusterctl/hack/create-local-repository.py as well?
@cwrau - with the open questions in the PR description, the first one is answered. How about the second question?
I guess we would add a dependency to the create-local-repository.py
script with a requirements.txt
, if no one minds that?