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

(2/3) Use Cobra, Koanf, and Ozzo rather than urfave/cli #114

Merged
merged 2 commits into from
Mar 18, 2023
Merged

Conversation

rtrox
Copy link
Collaborator

@rtrox rtrox commented Mar 17, 2023

This PR depends on #110 -- please review and merge first, and I will rebase this PR

Description of the change

Sorry, I know this is a huge PR! It would have been way more work to try and move to Cobra and Koanf separately. This PR attempts to make no functional changes, and only refactor to new libraries. The libraries in question:

  • Cobra (CLI)
  • Koanf (Configuration)
  • Ozzo (Validation)

Of course, I failed slightly at this -- I added Signal handling to the HTTP Server to shutdown gracefully when stopped.

Benefits

  • Better Separation of Concerns - config logic now all happens in config, config logic has been removed from client.
  • Simplified Command Flow - Each Command now only specifies the collectors which should run
  • New Libraries - closes Refactor: Switch to koanf & cobra for config & CLI library #58
  • Stronger Validation - ozzo provides well formed validation rules, to test for valid IPs, URLs, regex, etc with a simpler interface.

Possible drawbacks

It's just a big PR :-(

Applicable issues

Additional information

@rtrox rtrox requested a review from onedr0p as a code owner March 17, 2023 03:29
@rtrox rtrox changed the title Cobra koanf Use Cobra, Koanf, and Ozzo rather than urfave/cli Mar 17, 2023
@rtrox rtrox changed the title Use Cobra, Koanf, and Ozzo rather than urfave/cli (2/3) Use Cobra, Koanf, and Ozzo rather than urfave/cli Mar 17, 2023
@onedr0p
Copy link
Owner

onedr0p commented Mar 17, 2023

You are a machine! I am giving this a solid look thru but overall looks very solid!

My fingers are going to fall off typing up the release notes for v2.0.0 😄

@onedr0p
Copy link
Owner

onedr0p commented Mar 17, 2023

My only concern with using ozzo is its current maintained state.

go-ozzo/ozzo-validation#162

It's not a huge problem but I did see some more active libraries:

@rtrox
Copy link
Collaborator Author

rtrox commented Mar 17, 2023

oof, good catch! I totally missed that >.< I've been using it in other projects, looks like I have some updating to do :-D

Bummer that it's no longer maintained, it had a super clean interface. It looks like some folks are trying to revive it in a fork, but it was announced 3 weeks ago, which is coincidentally also it's last commit. Probably want to let that one bake a bit before counting on it.

I'll take a look at the interfaces of the libraries you linked and work on updating this.

@rtrox
Copy link
Collaborator Author

rtrox commented Mar 17, 2023

I just updated this to 1) rebase, and 2) move to gookit/validate rather than ozzo.

@onedr0p onedr0p merged commit 78c9991 into master Mar 18, 2023
@onedr0p
Copy link
Owner

onedr0p commented Mar 18, 2023

I spent some time reviewing the branch this morning and I'm very impressed!

@onedr0p onedr0p deleted the cobra_koanf branch March 18, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor: Switch to koanf & cobra for config & CLI library
2 participants