cluster-api icon indicating copy to clipboard operation
cluster-api copied to clipboard

✨ feature(clusterctl): Follow XDG Directory standard for config/data/... files

Open cwrau opened this issue 1 year ago • 16 comments

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 a dev-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

cwrau avatar Jul 13 '22 13:07 cwrau

CLA Signed

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:

k8s-ci-robot avatar Jul 13 '22 13:07 k8s-ci-robot

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.

k8s-ci-robot avatar Jul 13 '22 13:07 k8s-ci-robot

/ok-to-test

apricote avatar Jul 13 '22 13:07 apricote

@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.

k8s-ci-robot avatar Jul 13 '22 13:07 k8s-ci-robot

The apidiff is failing, but that is expected, what can I do?

cwrau avatar Jul 14 '22 07:07 cwrau

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 :)

killianmuldoon avatar Jul 14 '22 09:07 killianmuldoon

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 🙂

killianmuldoon avatar Jul 14 '22 09:07 killianmuldoon

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 👍️

cwrau avatar Jul 14 '22 09:07 cwrau

CLA has been accepted 👍️

/help

cwrau avatar Jul 27 '22 07:07 cwrau

Good to hear! I'm not sure what you're looking for with the help tag though :laughing:

killianmuldoon avatar Jul 27 '22 08:07 killianmuldoon

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 🤣

cwrau avatar Jul 27 '22 10:07 cwrau

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.

killianmuldoon avatar Jul 27 '22 10:07 killianmuldoon

Ah, ok, then I misunderstood the point of that label 😅

No, I can keep this PR 👍

cwrau avatar Jul 27 '22 10:07 cwrau

@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 avatar Aug 16 '22 12:08 sbueringer

@sbueringer

Ah, I see, yeah, I still need to have the questions answered 👍

By anyone, really 😅

cwrau avatar Aug 16 '22 16:08 cwrau

Got it.

cc @ykakarap I'll also take a closer look soon

sbueringer avatar Aug 16 '22 17:08 sbueringer

prior art from kubectl https://github.com/kubernetes/kubernetes/pulls?q=is%3Apr+XDG

dims avatar Aug 19 '22 23:08 dims

[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.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Sep 05 '22 11:09 k8s-ci-robot

cc @fabriziopandini

sbueringer avatar Sep 05 '22 12:09 sbueringer

@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.

k8s-ci-robot avatar Sep 13 '22 00:09 k8s-ci-robot

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

k8s-triage-robot avatar Dec 12 '22 01:12 k8s-triage-robot

/remove-lifecycle stale

cwrau avatar Dec 12 '22 08:12 cwrau

@cwrau Still interested in working on this?

vincepri avatar Jan 23 '23 20:01 vincepri

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?

richardcase avatar Mar 09 '23 08:03 richardcase

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 avatar Mar 09 '23 08:03 cwrau

@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.

killianmuldoon avatar Mar 09 '23 09:03 killianmuldoon

The tests that are failing are checks that verify that the path is $HOME/.cluster-api 😅

cwrau avatar Mar 10 '23 08:03 cwrau

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

richardcase avatar Mar 10 '23 12:03 richardcase

/retest

cwrau avatar Mar 10 '23 13:03 cwrau

/retest

cwrau avatar Mar 10 '23 13:03 cwrau

@cwrau - with the open questions in the PR description, the first one is answered. How about the second question?

richardcase avatar Mar 10 '23 15:03 richardcase

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?

cwrau avatar Mar 13 '23 08:03 cwrau