-
Notifications
You must be signed in to change notification settings - Fork 264
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
Conversation
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.
@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.
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 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
/retest |
As mentioned in the issue, one example is Helm, that moved to XDG spec.
I haven't found any reference for
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. |
To be more precise, there's a request for |
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, I suggest to bring this discussion forward to the WG call and doing a poll here. |
Quick Poll: Should we switch to |
Yes, it's the right time to do it now |
No, lets keep it in |
Ok let's move in the XDG direction. |
The following is the coverage report on the affected files.
|
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
/approve
[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 |
/test pull-knative-client-integration-tests |
* Create the CI flows for knative 0.5 * Update config.yaml
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:
Fixes #429
Proposed Changes
~/.config/kn
for unix-like OS%APPDATA%\kn
for MS WinXDG_CONFIG_HOME