Refactor configuration handling

[Darth-Mera]

This is a work in progress on refactoring the handling of configuration.

I have decided to scratch my first solution since it grew unnecessarily bloated and come up with something hopefully a bit more elegant.

Here I am: -using pflag package to have POSIX/GNU-style flags, both long and short versions -using Viper to introduce support for more config formats (I've played around with JSON and YAML, but it should also support TOML and some others) . Have to recognize that Viper is a big package, so if this is not desired, it should be quick and easy to revert back to the custom JSON unmarshalling, but my personal opinion is that having support of more formats is great. -proposing a change to the config structure so it's a bit more human-eye friendly

I have moved all the config handling logic to its separate package, exporting just a New() function that is called from main() to create a new config instance.

My plan is to also move DB connection logic there, and just pass pointers around the program.

I've also moved the global constants to its own package to avoid circular dependencies.

As it is WIP, there's currently only little validation or tests, I'm working on adding them.

PS: I realise that the DB logic is subject to change as Lev is implementing the multiple DB support - it will be changed accordingly later.

[lveyde]

Haven't reviewed it completely, since it's marked WIP, but I think that it's better to use flag package instead, as not to change the command line interface.

[alessandrocarminati]

My gut feelings are that having all these dependencies added for having the configuration in many formats is a bit more overkill. I don't think this tool will ever be certified for FUSA. It is helping FUSA experts for their analysis but since it is written in GOLANG, which comes with a non neglectable runtime, its certification is difficult. That been said, adding a fat dependency as viper is just for having the configuration possible with different formats does not add anything to its functionality and complicates its code base. For the same reason, I see adding dependencies for the flags parsing with caution, but I do see benefits in having more articulated parsing with respect to the one I made from scratches.

[alessandrocarminati]

In addition to this, obviously, but it is better to state it clearly, I found it very useful to move the configuration logic in a package so that it can be reused in the other tool. Finally, the tests are more than welcome. This topic was already discussed on other channels, but for transparency sake, it is better to write this here.

[rokkbert]

Haven't reviewed it completely, since it's marked WIP, but I think that it's better to use flag package instead, as not to change the command line interface.

But was it changed? Weren't there just things added for which no flags existed yet?

[rokkbert]

In addition to this, obviously, but it is better to state it clearly, I found it very useful to move the configuration logic in a package so that it can be reused in the other tool. Finally, the tests are more than welcome. This topic was already discussed on other channels, but for transparency sake, it is better to write this here.

After looking at the config here and in kern_bin_db I'm not sure how much reuse is possible, but separate package is cleaner in any case.

[rokkbert]

An important help/validation IMHO would be to tell the user exactly which mandatory arguments he's missing, instead of just "missing needed arg". I think that would fit well in here.

[Darth-Mera]

I have removed the Viper dep and rolled back to just supporting JSON config. However I would argue for keeping the pflag implementation, as it IMHO allows to implement what we want with less code and it's more readable.

As of now, this implementation works, but I will keep the PR as a WIP until the changes for multiple DB support are approved and I can implement them here as well.

[alessandrocarminati]

Sorry for the delay in reviewing your contribution, we had a few other things that needed our attention first.

Just wanted to give you a heads-up that I merged odra's contribution this morning, which conflicts with yours. Before I dive into the review, could you please rebase your work to the latest commit? Also, for future, it would be awesome if you could provide a squashed commit.

Thanks a lot for your help.

[Darth-Mera]

@alessandrocarminati

I can squash it to one commit, no problem, I have an experience that for reviewing it's better to see the chain of changes/commits, so I left the history, but I can clean it up.

Re rebasing: I've written my tests with testify, but the changes you've merged use ginkgo framework. So it seems there's some decision needed on the way we're supposed to test this tool.

[rokkbert]

LGTM now.