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

Remove ogier/pflag #193

Open
janisz opened this issue Mar 2, 2017 · 8 comments
Open

Remove ogier/pflag #193

janisz opened this issue Mar 2, 2017 · 8 comments

Comments

@janisz
Copy link
Contributor

janisz commented Mar 2, 2017

We are using ogier/pflag because it was introduced in original CiscoCloud project. pflag looks like abandoned (last commit over a year ago). We should switch to official golang flags. This change could be not backward compatible because ogier uses double dash -- while golang prefers - (see this comment)

@guilhem
Copy link
Contributor

guilhem commented Mar 6, 2017

Is it possible to switch to combo cobra / viper?
It's really powerful, simple to use and with a big community.

@guilhem
Copy link
Contributor

guilhem commented Mar 6, 2017

not to mention that, opposite to #195, we can be backward compatible

@janisz
Copy link
Contributor Author

janisz commented Mar 6, 2017

I made a small comparison between viper and flag. We try to minimize dependencies we are using in the code. Viper looks nice but we don't need most of it's features Changing from ogier to viper only to have live and supported project as a dependency is pointless. I hope reducing one dash (-) from flags won't be change that will cause an outage. We can remove ogier after we introduce SSE. So we will make two changes in config at once.

feature viper flag required by marathon-consul
setting defaults
reading from JSON, TOML, YAML, HCL, and Java properties config files
live watching and re-reading of config files (optional) ✗⁺
reading from environment variables ✗⁺
reading from remote config systems (etcd or Consul), and watching changes ✗⁺
reading from command line flags
reading from buffer
setting explicit values
"readability" of code (by removing the "config" object)
backward compatible
auto-generated documentation
simple usage

⁺ nobody requested it

@guilhem
Copy link
Contributor

guilhem commented Mar 6, 2017

So because nobody asks for a feature (which is just best practice) it's not a feature to have?
Strange mindset.
You also forgot about:

  • "readability" of code (by removing the "config" object)
  • backward compatible.

One big point I mention was the link with cobra which is really helpful for an app:

  • auto-generated documentation
  • simple usage
  • ...

Of course, everything is optional and an application can live without it... but it's just today right here, we have vendoring, why not using it "for free"?

@kamilchm
Copy link
Contributor

kamilchm commented Mar 6, 2017

Personally, I found https://github.com/jawher/mow.cli which works best for me. You could consider it if you like.

@janisz
Copy link
Contributor Author

janisz commented Mar 6, 2017

Thanks for response. Indeed I forgot about some features of cobra/viper that we might want. I'm not familiar with this tools so I'm not a fan of it. From my perspective ogier does nothing to help us and that's why I want to remove it. Adding new dependency (outside of stdlib) means somebody need to watch it and update when there are bugs to prevent users of marathon-consul to be hit by an error in our dependencies.

On the other hand, some features would be really nice:

  • configuration in YAML will allow having comments in config
  • removing config.Config struct
  • reading configuration from Consul
  • auto generated usage and readme

@wendigo
Copy link
Contributor

wendigo commented Mar 6, 2017

Combo Viper & Cobra looks nice :)

@janisz
Copy link
Contributor Author

janisz commented Mar 30, 2017

Anny update on this? Does anybody want to prepare PR with better configs handling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants