Skip to content
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

feat(XDG home): use XDG_CONFIG_HOME as default config location #668

Merged
merged 3 commits into from
Feb 28, 2020

Conversation

dsimansk
Copy link
Contributor

@dsimansk dsimansk commented Feb 14, 2020

Before proceeding further, I'd like to ask for feedback for the following change, especially to the deprecated fallback part and plugin dir handling.

@rhuss @navidshaikh
Does it seem like a right direction to go?

TODO:

  • Update docs about config location
  • Update help/usage messages with new default location
  • Add tests

Fixes #429

Proposed Changes

  • Change default config path to:
    • ~/.config/kn for unix-like OS
    • %APPDATA%\kn for MS Win
  • Respect env variable XDG_CONFIG_HOME
  • Check for pre-existing configuration dir on deprecated location and fallback to it. Otherwise use XDG recommended default.

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 14, 2020
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsimansk: 0 warnings.

In response to this:

Before proceeding further, I'd like to ask for feedback for the following change, especially to the deprecated fallback part and plugin dir handling.

@rhuss @navidshaikh
Does it seem like a right direction to go?

TODO:

  • Update docs about config location
  • Update help/usage messages with new default location
  • Add tests

Fixes #429

Proposed Changes

  • Change default config path to:
    *~/.config/kn for unix-like OS
  • %APPDATA%\kn for MS Win
  • Respect env variable XDG_CONFIG_HOME
  • Check for pre-existing configuration dir on deprecated location and fallback to it. Otherwise use XDG recommended default.

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.

@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 14, 2020
pkg/kn/core/root.go Outdated Show resolved Hide resolved
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced that XDG_CONFIG_HOME is the right thing to do. It deviates from kubectl and the rest of CNCF community AFAIK.

If kuebctl and others are also planning to change to this new fashion then I happy to stand corrected but would love to see the evidence (link to issue or PR).

Otherwise, I think it's ill-advised now to make this change. Especially since we have not reached version 1.0 and this will only confuse people.

My .02

@rhuss
Copy link
Contributor

rhuss commented Feb 17, 2020

/retest

@dsimansk
Copy link
Contributor Author

I am not convinced that XDG_CONFIG_HOME is the right thing to do. It deviates from kubectl and the rest of CNCF community AFAIK.

As mentioned in the issue, one example is Helm, that moved to XDG spec.

If kuebctl and others are also planning to change to this new fashion then I happy to stand corrected but would love to see the evidence (link to issue or PR).

I haven't found any reference for kubectl. Honestly, I'd say there'd be overwhelming push against it due to usage of cli itselft and handful of other tools that rely on current config location.

Otherwise, I think it's ill-advised now to make this change. Especially since we have not reached version 1.0 and this will only confuse people.

Well, I'd say that discussion and consensus should happen prior to reaching 1.0. It would be significantly more difficult to change location and provided meaningful backward-compatibility/migration after 1.0. Currently config is limited to pointing to plugin's dir + default plugin's dir and lookup strategy.
However, there's either WIP or feature-requests on third party plugin contribs, plugins provided by server etc. I'd say before 1.0 kn will produce a significantly more data on user's filesystem.
There's nothing wrong with current location, other than contributing to home polution that XDG is trying to address.

@dsimansk
Copy link
Contributor Author

To be more precise, there's a request for kubectl support, but without too much attention from SIG over 2 years and counting.

@rhuss
Copy link
Contributor

rhuss commented Feb 17, 2020

I also think that if we want to settle on that convention, we should do it now as we only 2 configuration values that are plugin related but we don't have any plugins contribution so I'm not sure if these options are really used.

Also, I think we should follow the XDG convention as it doesn't hurt us and for some people this is really important. I'm pretty sure, kubectl would settle on this, too, if there wasn't a huge backwards compatibility issue (that we don't have).

I suggest to bring this discussion forward to the WG call and doing a poll here.

@rhuss
Copy link
Contributor

rhuss commented Feb 18, 2020

Quick Poll: Should we switch to ~/.config/kn/ as default config location to play nicely with the XDG spec and help to keep the home directory clean ? (similar like Helm 3 does) (only 👍 are counting)

@rhuss
Copy link
Contributor

rhuss commented Feb 18, 2020

Yes, it's the right time to do it now

@rhuss
Copy link
Contributor

rhuss commented Feb 18, 2020

No, lets keep it in ~/.kn/

@rhuss
Copy link
Contributor

rhuss commented Feb 22, 2020

Ok let's move in the XDG direction.

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 26, 2020
@dsimansk dsimansk changed the title WIP: feat(XDG home): use XDG_CONFIG_HOME as default config location feat(XDG home): use XDG_CONFIG_HOME as default config location Feb 26, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 26, 2020
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/kn/core/root.go 50.4% 51.7% 1.3

Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, navidshaikh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2020
@navidshaikh
Copy link
Collaborator

/test pull-knative-client-integration-tests

@knative-prow-robot knative-prow-robot merged commit 034a9b3 into knative:master Feb 28, 2020
@dsimansk dsimansk deleted the issue-429 branch March 4, 2020 13:14
coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020
* Create the CI flows for knative 0.5

* Update config.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use XDG_CONFIG_HOME for location of the kn config
8 participants