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

cli: instantiate viper instead of using a singleton #119

Closed
wants to merge 1 commit into from

Conversation

BrunoRosendo
Copy link
Member

closes #76

This PR contains my try at implementing this issue but I don't think it's worth to change they way we handle viper. The issue was opened with the idea of simplifying tests, but currently this seems to make it harder. I think the only advantage of this would be to run tests concurrently, which we currently do not have.

As you can see, this approach has the following problems:

  • If we're instantiating viper once in root.go then we have to either pass viper to every single command or start the api client in root as well. I initially went with the second approach, since I thought the first one would make the code a lot more verbose without much benefit.
  • With the api being initiated before running any command, we have to possibly return errors to main without running the root command, which already breaks the flow of our client. To make it worse, this breaks the possibility of the user seeing the usage and version commands without setting the REANA_SERVER_URL env var.
  • That change also breaks the way our tests are being done. Right now, we're passing the right command to testCmdRun(), which will then perform all the tests necessary. Since now we have dependency injection, we'd need need either create a root command instead or pass a generation function to testCmdRun(). Neither of them work:
    • We can't pass a root cmd because we create a circular dependency between packages
    • We have different function parameters, since some commands need viper and others don't

The viable alternative seems to be passing viper to very single command. I can do it to show how it looks like but it doesn't seem worth it to me. I'm happy to hear your thoughts on this.

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.

cli: instantiate viper and swagger client in root command instead of using singletons
1 participant