#24 introduces a breaking change into the cherami library (for yarpc)

[willhug]

When I ran glide up in yarpc I started getting test/build errors because of new functions on the cherami consumer interface:

transport/x/cherami/mocks/mock_cherami_consumer.go:87: cannot use (*Consumer)(nil) (type *Consumer) as type "go.uber.org/yarpc/vendor/github.com/uber/cherami-client-go/client/cherami".Consumer in assignment:
        *Consumer does not implement "go.uber.org/yarpc/vendor/github.com/uber/cherami-client-go/client/cherami".Consumer (missing Pause method)
transport/x/cherami/mocks/mock_cherami_publisher.go:87: cannot use (*Publisher)(nil) (type *Publisher) as type "go.uber.org/yarpc/vendor/github.com/uber/cherami-client-go/client/cherami".Publisher in assignment:
        *Publisher does not implement "go.uber.org/yarpc/vendor/github.com/uber/cherami-client-go/client/cherami".Publisher (missing Pause method)

#24 introduced two new functions to the consumer interface. There hasn't been a release with these new changes (yarpc is still pointing to the master branch), but once a release goes out with this, all consumer implementations outside the cherami-client-go library will likely have build errors.

It's possible that yarpc is the only library depending on this interface, in which case this is probably ok, unfortunately if any other services/libraries depend on this interface people might start getting some weird build errors. If needed we can follow the semvar (http://semver.org/) rules to customize the versions we release.

cc: @breerly @abhinav

[prashantv]

In general, you should avoid overusing interfaces everywhere since it makes almost every change a breaking API change, forcing a new semver bump.

These articles cover why we shouldn't be overusing interfaces:
https://medium.com/@cep21/preemptive-interface-anti-pattern-in-go-54c18ac0668a
http://idiomaticgo.com/post/best-practice/accept-interfaces-return-structs/

[datoug]

This is only a problem for mock, right? If you prefer, we can bump up the version.
@prashantv Thanks for the links. I think there's a point there to return the actual struct to avoid the need to add unused mocks. However there's also a down side, which is every consumer now needs to define an interface for the structs in the library they consume (for a rich library like cherami client, that's a non-trival work for every consumer). I think a better idea probably is to keep returning interfaces, and also library owner provides mocks. The interface approach also servers as a contract, which is a plus I think. What do you think?

[prashantv]

@datoug From an API perspective, when you define an interface, there's no way to say "this is only for me to implement using my real client and my mock" -- any client code can implement the interface, so modifying the interface is a breaking change from a semver perspective and you would need to cut a new major.

Of course cutting new majors causes a lot of churn for service owners, so we should use types that reduce the amount of major revisions we need to make. Even if you provide a mock, modifying an interface is a breaking change.

There's a couple of techniques we've used to avoid using interfaces and avoiding breaking changes.

1. Instead of providing a mock client, provide a test server that sets up in-memory backend that can be controlled by the user. That way the user will always use the real client, but change the source of data to be this fake server. This leads to higher quality integrations since the user isn't mocking out "return a notFound error for this input" (which may not be true, since the real client returns a different error), but instead testing the real client.

2. Shim out the simplest underlying layer. E.g., if your client has 20 methods that use 3 API methods, then you may want to use an interface for the 3 methods (that is very unlikely to change), and the thick client just wraps this interface. This is similar to how Zap works, where zap.Logger is a struct, but zap.Core is an interface with very few methods and is unlikely to change.

[akshayjshah]

Was this releaseed? This is a breaking change - at the very least, it absolutely needs a major version bump. Including this as a minor degrades the utility of semantic versioning for all internal projects.

[datoug]

Thanks guys. I have bumped up the major version: https://github.com/uber/cherami-client-go/releases/tag/v2.0.0

[HelloGrayson]

You'll want cut a patch release on 1.x to revert any breaking changes.

[datoug]

@breerly The latest v1.* release doesn't have that change. Does this work for yarpc?
https://github.com/uber/cherami-client-go/tree/v1.20.1

[HelloGrayson]

You only need to make a patch release on 1.x if breaking changes made their way into 1.x.

[blampe]

I think we just ran into this again today, was this not reverted on the 1.x branch?

[HelloGrayson]

Yeah, we ran into this issue on this commit: 7a6f614

If 1.x is broken, then a new patch release needs to be put out that reverts the breaking change.

@datoug can you make that happen?

[datoug]

ack