Skip to content

Conversation

@rybit
Copy link
Member

@rybit rybit commented Aug 16, 2019

We were using an old library and the project has moved. I also cleaned up some of the config.

@rybit rybit requested a review from smoya August 16, 2019 17:53
Copy link
Contributor

@smoya smoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not in favor of having all configurations in the same package.
Instead, each package should know how to construct their own services from a config file (i.e.: nats.FromConfig(...).

By ensuring a clear separation of responsibilities and concerns we are:

  • Allowing packages to just pick up their real dependencies and do not carry on code that is out of their domain.

I.e.: Service "Greeter" only needs a logger from nconf. By having nconf package as dependency, code related to Nats is gonna be pulled as well.

  • Having a better code api as contexts are completely separated:
    logger, err := logs.FromConfig(...)
    natsClient, err := nats.FromConfig(...)
    VS
    logger, err := nconf.LoggerConfig{...}...
    natsClient, err := nconf.NatsConfig{...}...
  • Avoiding nconf package code base growing unnecessary far from it's responsibility and instead use it across other packages (let me borrow the DDD terminology of "Shared Kernel" bounded context) here).

@rybit
Copy link
Member Author

rybit commented Aug 20, 2019

Ok after talking with @smoya we decided that we'd go with this scheme:

  • config in the package is the default
  • if you can't (import loops, etc) then we move the whole project to use the central config package.

The goal is to have a consistent approach across the project. Not some packages with their own config and some in the conf package

Copy link
Contributor

@smoya smoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

@smoya smoya merged commit 765f492 into master Aug 23, 2019
@smoya smoya deleted the upgrade-nats branch August 23, 2019 08:26
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.

3 participants