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

Rehaul Configuration #407

Closed
zaquestion opened this issue Aug 27, 2020 · 14 comments · Fixed by #414
Closed

Rehaul Configuration #407

zaquestion opened this issue Aug 27, 2020 · 14 comments · Fixed by #414
Projects
Milestone

Comments

@zaquestion
Copy link
Owner

lab currently uses hcl as it's config format, and offers limited configuration. In large part minimal config options have been added to lab because hcl hasn't played super nicely with viper overall (particularly the automatic env vars functionality, which we want to leverage). For 1.0 I'd like to completely redo this, with a simpler file format that plays nice with automatic env vars. The main issue that arises has to do with ambiguity among file formats on if keys are implicitly treated as arrays or not, hcl has this issue, I think yaml may as well. So I suspect json is going to be a top candidate here. It's also possible that viper simply has better support for this in other formats (regardless of array considerations), so it's worth looking into yaml and ini (git config like option).

We aren't setup for this today, but in theory by using viper we should be able to read config from any number of formats. So we don't need to enforce what lab accepts, we should allow any viper formats to work (where reasonable / enabling ext suffices), but we should also settle on what the default is going to be go forward.

Lastly, out of the box this should provide improved (if not complete) support for #151 (multiple gitlab providers), this includes:

Related issues:

@zaquestion zaquestion added this to To Do in 1.0 via automation Aug 27, 2020
This was referenced Aug 27, 2020
@prarit
Copy link
Collaborator

prarit commented Aug 27, 2020

Personally I'd like to see an ini format but that's just me :). Let me look into this to see what I can come up with. To be clear, you have no objection to continuing to use viper. It's just that viper doesn't handle hcl very well, correct?

@rsteube
Copy link
Collaborator

rsteube commented Aug 27, 2020

Big fan of yaml personally, but toml is prefered by a lot for it's more unambiguous syntax (it's also pretty similar to ini).
I find json not so great for configs that are edited by hand.

@zaquestion
Copy link
Owner Author

zaquestion commented Aug 27, 2020

To be clear, you have no objection to continuing to use viper. It's just that viper doesn't handle hcl very well, correct?

Correct, and more specifically, I want automatic environment vars to work more or less out of the box and not require all manner of shennanigans to work https://github.com/zaquestion/lab/blob/master/main.go#L54-L137

Had forgotten what toml looked like when I quickly wrote this up this morning. Agree toml seems like the stronger route (over ini) to supporting "git like" configs.

@prarit
Copy link
Collaborator

prarit commented Aug 29, 2020

Big fan of yaml personally, but toml is prefered by a lot for it's more unambiguous syntax (it's also pretty similar to ini).
I find json not so great for configs that are edited by hand.

Ditto and I think we're all in agreement that toml looks like the best approach moving forward.

@prarit
Copy link
Collaborator

prarit commented Aug 29, 2020

I would like to implement something that differentiates between a local and global config, just like git does. For example, if there is a local .git/lab/config, the settings in that file would override the ~/.config/lab/config settings. Doing that, however, increases complexity as there would have to be a 'lab config' command that had a --local and/or --global option.

An example: .git/lab/config contains
[gitlab.com]
token 12345abcde
user prarit

that overrides the ~/.config/lab/config

[gitlab.com]
token 67890vwxyz
user john.doe

The config code would always default to searching global first, and then overriding with the local values.

@prarit
Copy link
Collaborator

prarit commented Aug 29, 2020

Another reason we want to have global and local configs: On some projects I'm going to want the comments always displayed when doing a 'show mr', and on other projects I'm likely not going to care unless I really want it. Having a

[mr show]
comments = true

would easily fix that type of problem.

@prarit
Copy link
Collaborator

prarit commented Sep 2, 2020

A few things to report. I don't think the HCL code is working at all. viper.GetString("core.token"), for example, is always empty.

I've filed

spf13/viper#967

to see if there is any feedback. I've tried YAML (which works!), and HCL & TOML (both of which fail).

@prarit
Copy link
Collaborator

prarit commented Sep 2, 2020

My plan is to:

  1. Convert the existing viper infrastructure to use TOML instead of HCL. This requires a new config function ConvertHCLtoTOML that would convert existing HCL files to TOML. It would be run from main().

As noted in the previous comment, however, spf13/viper#967 is a problem. It could be that I just need a gentle tap on the head with a cluebat ;) and there is some subtle nature of using the other formats that I don't understand.

  1. Update the existing viper infrastructure to always override when using environment vars LAB_CORE_HOST, LAB_CORE_TOKEN, and LAB_CORE_USER. This should be straight forward.

  2. Keeping in mind Documentation for configuration #2, update the existing order of ./, ~/.config/, and .git/ to be

.git/lab
~/.config/lab
user specified config file (--configfile --conf --config?)

This config detection order would allow users to specify a per-tree host, token, and user and easily override the global ~/.config/lab entry.

While this is written focusing on the core entries (host, token, and user), it should be noted there are other config entries of tls.skip_verify and tls.ca_file. I am aware that these configs also need to be handled appropriately.

@bmeneg
Copy link
Collaborator

bmeneg commented Sep 2, 2020

May I also suggest to move the configLoad() code into internal/config/config.go as Load() (or something similar)?
Thus we have a clear separation of configuration organization related code and its usage.

@prarit
Copy link
Collaborator

prarit commented Sep 2, 2020

I have the first part of the plan above staged in #414.

@prarit
Copy link
Collaborator

prarit commented Sep 2, 2020

  1. Use ~/.config/lab/lab.toml <- according to a colleague it is bad practice to leave a config in the .config directory.

@prarit
Copy link
Collaborator

prarit commented Sep 2, 2020

The second part is also staged in #414

@zaquestion
Copy link
Owner Author

Use ~/.config/lab/lab.toml <- according to a colleague it is bad practice to leave a config in the .config directory.

yeah agreed, would be best to put under it's own directory

@bmeneg
Copy link
Collaborator

bmeneg commented Sep 2, 2020

Both first and second parts are looking good in #414! :)) Just had a couple of simple doubts, which I added in the PR.

And about a lab/ folder for the config file... +1.

@prarit prarit linked a pull request Sep 4, 2020 that will close this issue
1.0 automation moved this from To Do to Done Sep 6, 2020
@zaquestion zaquestion added this to the 0.18.0 milestone Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
1.0
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants