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

v3.2.0-rc.0 compatability #7851

Closed
mwf opened this issue May 2, 2017 · 6 comments
Closed

v3.2.0-rc.0 compatability #7851

mwf opened this issue May 2, 2017 · 6 comments
Milestone

Comments

@mwf
Copy link

mwf commented May 2, 2017

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.

@heyitsanthony
Copy link
Contributor

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.

heyitsanthony pushed a commit to heyitsanthony/etcd that referenced this issue May 2, 2017
@heyitsanthony heyitsanthony added this to the v3.2.0 milestone May 2, 2017
@mwf
Copy link
Author

mwf commented May 2, 2017

Nothing will fail in the runtime here, because you still close the keepalive channel in case of the error.
So the worst thing could possibly happen - people won't log the real 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 :))

@heyitsanthony
Copy link
Contributor

OK, I'm kicking this back to 4.0.0 to comply with semver.

@mwf
Copy link
Author

mwf commented May 2, 2017

Oh... I'm really waiting for fixing this KeepAlive thing, so I was happy you've done it :)
4.0.0 looks like forever :(

Maybe there is any way to keep it in 3.2 with the compatability fixes? Let's ask your teammates' opinion?

@heyitsanthony
Copy link
Contributor

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.

@mwf
Copy link
Author

mwf commented May 2, 2017

Looking forward for 4.0.0 then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants