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

~/.kn/config.yaml should accept and retain camel cased configuration #414

Closed
maximilien opened this issue Sep 23, 2019 · 4 comments · Fixed by #428
Closed

~/.kn/config.yaml should accept and retain camel cased configuration #414

maximilien opened this issue Sep 23, 2019 · 4 comments · Fixed by #428
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.
Milestone

Comments

@maximilien
Copy link
Contributor

In what area(s)?

Kn configuration. Also in docs.

See PR #411 for discussions

Classifications:

/kind cleanup

What version of Knative Client?

maximilien@maxmini:$ ./kn version
Using kn config file: /home/maximilien/.kn/config.yaml
Version:      v20190920-local-94bb1cf-dirty
Build Date:   2019-09-20 20:38:29
Git Revision: 94bb1cf
Support:
- Serving: v0.8.0  v0.7.1
- API(s):  v1alpha1

What version of Knative Serving running on your cluster?

0.8.x

Expected Behavior

Configuration entries in the ~/.kn/config.yaml file should retain their case sensitivity.

Actual Behavior

Currently they seem to always be caseless, e.g., pluginsdir: ~/.kn/plugins

Steps to Reproduce the Problem

Start without any ~/.kn/config.yaml and create emty file touch ~/.kn/config.yaml.
Run kn --help
Your ~/.kn/config.yaml will be populated with default configurations that are caseless.

@maximilien maximilien added the kind/bug Categorizes issue or PR as related to a bug. label Sep 23, 2019
@maximilien
Copy link
Contributor Author

/assign @maximilien

@maximilien
Copy link
Contributor Author

maximilien commented Sep 30, 2019

OK so after about an hour or so of digging, it seems that Viper does not have support to keeping the keys in the config case sensitive. This is a know issue and problem that the Viper community have been debating for over a year.

A PR was submitted earlier this year to add support to Viper to allow end user to select case sensitivity or not. There is much discussions and the PR author has kept it up to date. However, it has not been merged. This has cause some users to move on and use other config system, fork Viper to include PR, or build their own. I could not see a clear way out but I have not digged deep yet.

So the question to all is what to do about this. I see three possible outcome:

  1. We are OK with the current case non-sensitive behavior for keys.

I have checked that Viper does indeed keep case for values, so if the user set pluginsDir: ~/.kn/Plugins Viper will read it correctly. The consequence is how it behaves now, configuration have their keys flatten, so for us:

~ cat ~/.kn/config.yaml
lookuppluginsinpath: true
pluginsdir: ~/.kn/plugins

And when / if Viper adds case sensitivity we can turn that on then.

  1. Build our own. The code for Viper is not much and since a lot of the features that it provides are not used, we could easily build our own config loading/saving that dealt with YAML and our config. I have done similar subsystem in the past---as I am sure others have too.

  2. Investigate using different configuration system / libraries. I have not gone that route yet. But I could spend a day on this. My fear is bringing in more dependencies...

I'll add an item on the 10/01/2019 working group where we can hopefully decide so I can resolve this.

@maximilien
Copy link
Contributor Author

maximilien commented Oct 1, 2019

After today's 10/01/2019 call we agreed to go with option 1 above ^^^ and also use dashes in the keys. So the example config above would become:

~ cat ~/.kn/config.yaml
lookup-plugins-in-path: true
plugins-dir: ~/.kn/plugins

There is also interest in simplifying the names for keys (yeah). Since we have two keys and plugins-dir is not too long the key ask is to simplify lookup-plugins-in-path. Suggestions welcome? I personally would be happy with: lookup-plugins since the PATH part is implied a bit. Please chime in your thoughts / suggestions here.

maximilien added a commit to maximilien/client that referenced this issue Oct 1, 2019
This solves 414 by adopting new format for config keys and thereby
bypassing the need for case sensitivity in Viper. Also changed
`lookupPluginsInPath` key to `lookup-plugins` and also same for the
persistent flag. The PATH part is implied and can be read from
help.
@maximilien
Copy link
Contributor Author

OK let's move all discussions to PR #428

maximilien added a commit to maximilien/client that referenced this issue Oct 3, 2019
This solves 414 by adopting new format for config keys and thereby
bypassing the need for case sensitivity in Viper. Also changed
`lookupPluginsInPath` key to `lookup-plugins` and also same for the
persistent flag. The PATH part is implied and can be read from
help.
knative-prow-robot pushed a commit that referenced this issue Oct 4, 2019
This solves 414 by adopting new format for config keys and thereby
bypassing the need for case sensitivity in Viper. Also changed
`lookupPluginsInPath` key to `lookup-plugins` and also same for the
persistent flag. The PATH part is implied and can be read from
help.
maximilien added a commit to maximilien/client that referenced this issue Oct 8, 2019
…ase (knative#428)

This solves 414 by adopting new format for config keys and thereby
bypassing the need for case sensitivity in Viper. Also changed
`lookupPluginsInPath` key to `lookup-plugins` and also same for the
persistent flag. The PATH part is implied and can be read from
help.
@navidshaikh navidshaikh added this to the v0.8.0 milestone Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants