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

Breaking up the config and pubsub packages by dependency #62

Merged
merged 13 commits into from
Jun 15, 2016

Conversation

jprobinson
Copy link
Contributor

One of the largest of complaints against Gizmo so far has been how the toolkit lumps all dependencies into the config and pubsub packages, often requiring users to vendor or fetch an absurd amount of packages just to work with a simple server.

This PR hopes to fix this problem by breaking out the config and pubsub packages into a set of subpackages, which follows a similar structure to Go Kit and it's implementations. The new package structure:

├── config
│   ├── aws
│   ├── combined
│   ├── cookie
│   ├── gcp
│   ├── metrics
│   ├── mongodb
│   ├── mysql
│   └── oracle
├── pubsub
│   ├── aws
│   ├── gcp
│   ├── kafka
│   └── pubsubtest
├── server
└── web

Notable changes:

  • config.Config is now config/combined.Config
  • config.Server is now server.Config
  • If a "config" type got moved to a package with the same name, it is now likely renamed as Config.
    • example: config.MongoDB moved to config/mongodb.Config

@coveralls
Copy link

coveralls commented Jun 13, 2016

Coverage Status

Coverage decreased (-2.4%) to 42.471% when pulling f8b1879 on config-breakout into 46437ab on master.

@basvanbeek
Copy link

basvanbeek commented Jun 15, 2016

just a quick browse. I notice there are now multiple different logging packages found (log, kit/log and Sirupsen/Logrus and sometimes a global var initializing for instance Logrus. (see pubsub.go)

If you need to support these various loggers I would suggest building a logger interface which each package can take as a functional option. This way one can write a wrapper around the various logging packages to implement the interface and give that to the gizmo packages... now everybody can keep on using their preferred logging system.

pssst If you want to move to a single logging package.... Go kit ;)

@jprobinson
Copy link
Contributor Author

Thanks for looking things over, @basvanbeek!

The plan is move towards the one logger, but I couldn't think of any better, small solution at the time of the metrics PR. In the near future, I'll put together a logging PR that will clean things up a bit.

This log problem you mention was here before this PR, though (it was introduced in #59). Did you have any comments on anything new introduced here? If not, I'm going to merge in the next couple hours.

@basvanbeek
Copy link

just a couple of minor items which are more in the realm of opinions :)

I would for instance remove global variables (example RequiredAcks = saram.WaitForAll in pubsub/kafka/kafka.go) and have them become consts holding the default values for these items.

Have explicit rather then implicit changes to behavior by functional options to constructors or explicitely set values from your config objects.

@jprobinson
Copy link
Contributor Author

I actually like that approach and hope we can fold in some of the changes from your fork once we have this merged into master. PRs welcome :)

Conflicts:
	config/gcp.go
	pubsub/gcp.go
@coveralls
Copy link

coveralls commented Jun 15, 2016

Coverage Status

Coverage decreased (-2.7%) to 42.471% when pulling d32a935 on config-breakout into f908f58 on master.

@coveralls
Copy link

coveralls commented Jun 15, 2016

Coverage Status

Coverage decreased (-2.7%) to 42.471% when pulling dec1e76 on config-breakout into f908f58 on master.

@jprobinson jprobinson merged commit 9189d2b into master Jun 15, 2016
@jprobinson jprobinson deleted the config-breakout branch June 15, 2016 18:21
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.

None yet

3 participants