streadway / amqp

Go client for AMQP 0.9.1
http://godoc.org/github.com/streadway/amqp
BSD 2-Clause "Simplified" License
4.88k stars 621 forks source link

go modules support #388

Closed kpurdon closed 5 years ago

kpurdon commented 5 years ago

Would you be interested in supporting Go Modules. This would entail:

  1. Generating a go.mod and go.sum in the root of this repo.
  2. Adopting semver and releasing versions (not explicitly required, but highly recommended)
  3. Updating test running for modules support.

I'm more than happy to open a PR with these changes, but would like to discuss them here before doing so!

emirb commented 5 years ago

👍 for adopting semver. Previously it was closed in https://github.com/streadway/amqp/issues/312

michaelklishin commented 5 years ago

I suspect it entirely depends on what the answer to #312 is today. @streadway are you open to discussing this once again?

streadway commented 5 years ago

I'm happy to discuss the value proposition of any change, including Go Modules or SemVer. Please make a case supported with examples.

The current case I'm working with is personal preference for x.y.z labeling over date+sha labeling, so it would be easier to present and discuss the pros and cons of something other than preference with a maintenance task or regression as an example.

kpurdon commented 5 years ago

@streadway thanks for the response. I think the biggest case I can make for Modules & SemVer is simply that the Go team, community, and toolchain has finally rallied around them as the solution to versioning and dependency management. For that reason alone I think it's worth adopting.

To add some color to that statement:

  1. Go officially recommends using SemVer https://github.com/golang/go/wiki/Modules#semantic-import-versioning
  2. Go modules are the future (the future is now!) of Go dependency management

This repo is one of the simplest cases for Modules (no external deps, no existing releases). The work required will be: add a go.mod, release v1.0.0 as a tag, and a minor update to TravisCI tests to use modules mode for Go 1.11+.

Having SemVer tagged releases will also allow for any potential future breaking changes that could come. Though unlikely, it's nice to have the flexibility.

Thanks for all the work on this package, and again I'm more than happy to pull the modules PR together whenever you give the 👍.

michaelklishin commented 5 years ago

For a library that's into its 7th year breaking changes are not also likely, they are necessary since all the inevitable mistakes in API design are now a lot more obvious than in the early days. This is even if we ignore all the changes naturally coming from the Go side.

streadway commented 5 years ago

What I hear is that breaking changes are desired and the import-versioning recommendation suggests using a different module name instead of using tag. This effectively forks the repo using different branches for backporting bug fixes into different major versions.

If the module is version v2 or higher, the major version of the module must be included as a /vN at the end of the module paths used in go.mod files (e.g., module github.com/my/mod/v2, require github.com/my/mod/v2 v2.0.0) and in the package import path (e.g., import "github.com/my/mod/v2/mypkg").

I am fine with adding a go.mod which simply contains the package name to track HEAD (which is basically what we do today). We don't yet have a release process for tagged commits documented for our contributors, so the process and value of tagging commits is still unclear. Documenting the release process is a different issue.

Instead of talking about tagging and go.mod (which is easy) - let's discuss what the desired breaking changes are and whether they're valuable enough to fork the releases into 2 modules.

@michaelklishin could you please lead an amqp/v2 issue to collect a roadmap for API improvements? I will have very little time to work on a v2, so we need to be realistic that the breaking changes would be implemented, supported and valuable enough to warrant a fork of this package into a new major version.

streadway commented 5 years ago

WDYT about documenting the release process and v2 milestone first?

alexrudd commented 5 years ago

Maybe I'm misunderstanding, but what is the need for forking this repository to work on a v2 API?

If the current state of master was tagged as v1, then anyone who wanted to lock their imports to v1 can do so in their own go.mod file. v2 can be developed on its own branch and then released into master when it's ready.

Also adding my general support for go modules - that's actually why I came here to see if support was being worked on :)

streadway commented 5 years ago

Maybe I'm misunderstanding, but what is the need for forking this repository to work on a v2 API?

This isn't a "Github fork", but rather addressability of the modules and maintaining more than one HEAD reference for each module. The recommendation is to use different module names for different major versions.

We'd keep a v1 branch that took bugfixes and v1 related improvements, and a v2 branch with a different API that would take similar bugfixes and v2 related improvements. The commits for each HEAD will continue to reside in this git repository.

streadway commented 5 years ago

v2 can be developed on its own branch and then released into master when it's ready.

What impact would merging v2 back into master have for dependents on the v1 API that do not have a go.mod, or haven't added a require statement in their go.mod?

alexrudd commented 5 years ago

This isn't a "Github fork"

I see. My experience of Go modules is mostly from the consumer side.

What impact would merging v2 back into master have for dependents on the v1 API that do not have a go.mod, or haven't added a require statement in their go.mod?

Assuming they haven't vendored, it would be a breaking change; but that's the risk they have been taking by not managing dependencies. Though I can see why that's a legitimate problem for such a widely used package as this.

michaelklishin commented 5 years ago

@streadway I'd be happy to do it but I need to understand the plan. I still don't fully understand why there can't be a single repo with multiple branches, that's what every other client does. API changes every few years are seen as reasonable by the vast majority of the industry (or at least its part that I interact with). This client and the protocol it implements have been remarkably stable over the years. Clients from 2012 still can connect and use the majority of the features.

Why not move default branch to 1.x and retain the current API, announce this change and in a few months begin integrating and documenting changes and differences from 1.x. I have been doing something similar successfully for years with other clients; that's how RabbitMQ and tier 1 plugins are developed, too.

I understand that some users would never see any announcements, would not act on it but if it's a matter of changing a branch, honestly, it's their problem.

michaelklishin commented 5 years ago

An alternative solution which I don't like is to fork this project as in GitHub fork, change package name, move it to the RabbitMQ org and adopt the practices used for other clients and RabbitMQ itself. Current users of this package will then continue using this plugin which will only get non-breaking fixes.

I expect this to cause a lot of confusion initially and, once RabbitMQ tutorials switch to the "new client", this plugin will eventually get little attention.

I don't like this on many levels, especially given that we don't plan any drastic changes to the API. Java and .NET clients have undergone more significant changes in the last couple of years, and more are planned. The community response has actually been very positive because everyone wants to use .NET Core, latest Java language features and so on with these clients.

michaelklishin commented 5 years ago

Sorry that I hijacked this conversation but I feel we need to figure out a multi-release series strategy first and then see how Go modules would fit, even if it's the inevitable future for the Go ecosystem. @AlexRudd looks like we are suggesting something similar, would you agree?

lukebakken commented 5 years ago

This comment and this comment pretty much describe the version and branching workflow used in the Pika project with regard to version 0.13.0 (stable) and the upcoming 1.0.0 release (master).

For this project, at some point go.mod can be added to master, and tagged v1.0.0. Fixes would then follow semantic versioning. When a backwards-incompatible change is made at some point in the future on master, then the v2 name is added to the module name, and the previous commit becomes the base for future fixes for v1 versions. Fixes on the v1 branch (whatever it's called) can be forward-merged into master as needed. Anyway, my $0.02

alexrudd commented 5 years ago

WDYT about documenting the release process and v2 milestone first?

We got a bit off-track. So that's formalising a potential release process using go mod, and proposing API changes that would justify a v2 release?

I would be interested to hear of anyone's experiences doing regular releases of a go module. Also any examples of other repos that are doing this well.

gerhard commented 5 years ago

I don't see the need to discuss breaking changes - and thus v2 - in this context.

My understanding of Can a module consume a package that has not opted in to modules? makes me think that all we need is just git tag v1.0.0. If we don't do this, when this package is required in a module, it will end up with a pseudo-version such as v0.0.0-20171006230638-a6e239ea1c69

Once we decide to introduce v1 and therefore commit to a stable API (this is a foregone conclusion), we may want to discuss breaking changes. If we find ourselves in that position, and we have a v1 - we currently don't - we have the option of taking features on a case-by-case basis and decide at that point in time whether they merit a major version or not. With no semver, we can't even begin to have those conversations, which I perceive as frustrating for some.

Did I oversimplify?

streadway commented 5 years ago

I emphasize backwards compatibility in this library for client application simplicity (follow head, get fixes), and maintainer simplicity (develop on head, roll forward for fixes). I value client application simplicity much more than maintainer simplicity so I'm willing to adopt any release process that makes receiving updates to this library easier for application developers.

It may be worthing elaborating on my opinions. I think of dependency management in 2 ways: continually integrate changes for development, or ensure build repeatability for release. We don't need SemVer for either as long as different (backwards incompatible) APIs have different names. I believe that version pinning makes receiving updates from dependencies like this library harder. Without versioning, it's one less thing to worry about. ">=1.0.0" is what this library does by default without the incidental complexity of understanding and following the version number ceremony.

At work, we use go modules but we also want to accept upstream changes rapidly. To continuously integrate, we use an internal tool like https://dependabot.com/ that creates PRs to increase the version for every upstream change. I have hope that disciplined maintainers could have provided an alternative to the amount of version churn application development is faced with, just to keep up.

For release repeatability, we save the built application at a test version, rather than try to save all the versions of all the dependencies. This means if we want to repeat a release, we already have the artifact and don't need to track sources.

With go modules, it's documented how both the client and maintainer are expected manage v2+ changes. Following those expectations makes my opinion moot, and that makes things simpler in the long run for both future maintainers of this library and existing application developers.

streadway commented 5 years ago

With no semver, we can't even begin to have those conversations, which I perceive as frustrating for some.

We can (and should) have these conversations without SemVer - we just would use a different import path instead of a git tag.

alexrudd commented 5 years ago

The cargo cult of versioning

An interesting read, thanks for linking!

So the approach that seems to best serve all those downstream would be to add a go.mod file to the master branch for module github.com/streadway/amqp, and then if there are compelling reasons to work on a v2 of the API: branch off of master, update go.mod to module github.com/streadway/amqp/v2 and release it as v2.0.0 when it's ready.

Does that sound right?

streadway commented 5 years ago

So the approach that seems to best serve all those downstream would be to add a go.mod file to the master branch for module github.com/streadway/amqp, and then if there are compelling reasons to work on a v2 of the API: branch off of master, update go.mod to module github.com/streadway/amqp/v2 and release it as v2.0.0 when it's ready.

Yeah that sounds about right. I think we can add go.mod today without any tags.

kpurdon commented 5 years ago

Based on all the conversation here I've opened #394 which simply adds the go.mod and updates TravisCI to test in module mode for Go 1.11+.