-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
v3.2.0-rc.0 compatability #7851
Comments
It changes the underlying behavior of that API. The logic change can be done without interfering with the API so code still compiles, but the error handling contract will be very different. The choice is either fail it statically at compile time or wait until something depending on the old behavior fails dynamically at runtime. |
This reverts commit fbbc4a4, reversing changes made to f254e38. Fixes etcd-io#7851
Nothing will fail in the runtime here, because you still close the keepalive channel in case of the error. I totally agree that in case of having a failed build you will immediately pay attention to it and read the notice then. But what about the semver? Either you follow it or not... I understand you feelings here - you want to have the code clean, you want to notify 100% of users with this change and you don't want to increment the major, cause having etcdv3 server and 4.x.x version is clumsy. But I don't think people really catched the bug with KeepAlive like me in #7472. So either they don't care about error handling or they just don't care about this situation :) It's your choice how to maintain you library, but in my opinion we should respect the semver agreement. Because otherwise it brings pain to users and spoils your Carma :)) |
OK, I'm kicking this back to 4.0.0 to comply with semver. |
Oh... I'm really waiting for fixing this KeepAlive thing, so I was happy you've done it :) Maybe there is any way to keep it in 3.2 with the compatability fixes? Let's ask your teammates' opinion? |
The contract has to change for what you're asking. It would be compile compatible, but the /semantics/ are still different. This breaks the spirit of semver. Sorry. |
Looking forward for 4.0.0 then :) |
Guys, I'm concerned about breaking backwards compatability in v3.2.0-rc.0 release.
You should avoid doing breaking changes in minor version update (see http://semver.org/).
You could avoid breaking changes in #7488 just having
error
in the method interface and always returning nil. As you did in 3.0.x BTW :)And #7305 does not break API at all, but just some logic. So it's safe to keep it in the release as is.
Having "Breaking changes" notice in the release notes is awesome, but let's keep the compatible API? So that users could rely on semver policy and use proper wildcards in their vendoring tools.
The text was updated successfully, but these errors were encountered: