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 API Change #66

Open
packrat386 opened this issue Jan 3, 2018 · 57 comments
Open

Breaking API Change #66

packrat386 opened this issue Jan 3, 2018 · 57 comments

Comments

@packrat386
Copy link

0ef6afb breaks the API that's been stable for years. Why is this important enough to change with no warning, despite the community standard to be avoiding breaking API changes without some kind of versioning system? The original issue indicates some sort of deprecation warning was considered, but seems to never have been implemented. If users are going to have to rug pulled out from under them with no warning, why should we use this library?

@markbates
Copy link

I just hit the same problem with github.com/markbates/pop and github.com/gobuffalo/buffalo this breaks both projects.

I agree with @packrat386 this is a big breaking API change that was sprung on everyone.

Can we please revert that change, for now, and create a path to making the breaking change?

@jjuarez
Copy link

jjuarez commented Jan 3, 2018

I have faced the same issue

@packrat386
Copy link
Author

packrat386 commented Jan 3, 2018

We've already fixed it in our code, so honestly undoing it is only going to cause more problems for us, but either way this approach to API stability is a major problem. This should have been NewV1WithError() or something.

EDIT: I understand that reverting might be the best solution, just pointing out that in some cases the damage is already done.

@sbowman
Copy link

sbowman commented Jan 3, 2018

At this point if you want to change the API, you should create a new project or version it somehow. Your change basically broke a dozen projects at our company.

@davidpope
Copy link

davidpope commented Jan 3, 2018

Agreed with @sbowman , this broke a bunch of our projects.

If you use version tagging, at least those of us using glide and 'go dep' can protect ourselves by pinning our major versions. For example, if you tag f58768 as 1.2.0 and 0ef6af as 2.0.0, the fix is simple on our end.

I agree that the proper immediate fix is to revert, however.

Edit: For clarity, we have already protected ourselves by pinning to the latest August version (0ca0c2). I'm suggesting tagging here simply as a good pattern to follow.

@prowley
Copy link

prowley commented Jan 4, 2018

Also broke the goa project.

@glerchundi
Copy link

I agree with you all that these kind of breaking changes should be treated in a much more soft way. BUT, consider two things:

  • It's a personal project and he can do whatever he wants,
  • If you're pinning to master branch, you're already doing it wrong.

@jinroh
Copy link

jinroh commented Jan 4, 2018

This is also an issue for us because we rely on the dep chain hermes -> sprig -> go.uuid.

Could we introduce another method instead of making a breaking change, like uuid.NewV4Safe() or something along those lines ?

@ivucica
Copy link

ivucica commented Jan 4, 2018

Original request to make the change that broke people, #18, talked about "let's warn people for 6-12mo" which I don't think happened.

@packrat386
Copy link
Author

#18, talked about "let's warn people for 6-12mo" which I don't think happened.

I think that's the important bit here. If there was something like a deprecation warning or a new method, I don't think anyone would be opposed to the change. I, for one, certainly prefer a library that returns an error rather than panicking. The issue isn't with the content of the change, but with how it was rolled out.

@thurt
Copy link

thurt commented Jan 4, 2018

regarding how to handle the error, is this error merely transient? ie. can we simply keep retrying to create UUID until we do not get an error?

@jasonkeene
Copy link

Software changes.

(•_•)
( •_•)>⌐■-■
(⌐■_■)

Deal with it.

@ivucica
Copy link

ivucica commented Jan 4, 2018 via email

@ktnyt
Copy link

ktnyt commented Jan 4, 2018

I personally want to support @thurt's suggestion about looping until the method succeeds for uuid.V*. As @jinroh and others pointed out functions that return multiple values should have been implemented separately. I think this project has grown popular to the point where destructive changes in the interface can no longer be excused for the project being personal. Sure it's no Python 3 but still a lot of people, myself included, love and rely on this amazing piece of work.

@evillemez
Copy link

A version tag pre/post this breaking change would be helpful. In the meantime, I'll try and pin to a specific commit before the change.

@cstockton
Copy link

cstockton commented Jan 4, 2018

There are many options besides committing a breaking change and causing months of collateral damage. If you want a good example of what happens when a project has a breaking change look at sirupsen/logrus#451 - as projects slowly update their code you end up with some dependencies fixed, some broken for many months. It's most painful for the people who import the broken packages (like me, importing osrg/gobgp), because I have no control over when it is fixed.

The best course of action is to revert and create a proper version and import path for those who ARE in the position to update and wish to do so. This will also allow both versions to live side by side. Please don't leave all these projects dealing with random interruptions as various projects update over the next N months :/

EDIT: To be clear the faster it is reverted the least damage done to the ecosystem. The people who already fixed their changes are clearly agile enough to revert and it's fresh on their mind so they will know exactly where to look. The people who have not yet had to research this will never have to. Leave the change and EVERYONE who uses this library will eventually be affected.

@evillemez
Copy link

evillemez commented Jan 4, 2018

For those using dep, I changed my Gopkg.toml to this, which pins to the last commit on Jan. 2, the day before the change:

[[constraint]]
  name = 	"github.com/satori/go.uuid"
  revision = "063359185d32c6b045fa171ad7033ea545864fa1"

Not the best solution, but unbreaks things for now.

EDIT: Use [[override]] instead of [[constraint]] if it's a transitive dependency: https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md#override

@ktnyt
Copy link

ktnyt commented Jan 5, 2018

Please let me apologize in advance for jumping the gun here but I was starting to get the impression that reverting the interface was pretty urgent. I tried not to destroy the modified interface as much as possible while reverting the global namespace NewV* functions (which I believe is the heart of most issues) to the original function signatures. If the NewV* methods for the Generator interface needs to be reverted there is a whole lot more to do (which might be better off just completely reverting the past few commits).

@qct
Copy link

qct commented Jan 5, 2018

though breaking changes bring tons of damage to developers, but I want to say why don't you use release version, like v1.1.0?

@ivucica
Copy link

ivucica commented Jan 5, 2018 via email

alexdor added a commit to alexdor/gocelery that referenced this issue Jan 5, 2018
Package satori/go.uuid changed the api for generating uuid, more info here:
satori/go.uuid#66 . This commit handles the breaking changes without
changing any of the public facing apis
@grugnog
Copy link

grugnog commented Jan 5, 2018

Also seeing fails on github.com/Masterminds/sprig and github.com/qntfy/kazaam

@mattwilliamson
Copy link

mattwilliamson commented Mar 28, 2018

It could have easily been done by adding a new function instead of breaking tons and tons of projects. I, myself have about 20 that were with 8 developers on my team -- 160 changes not including laptops. Switching to github.com/google/uuid was faster and easier than implementing error checking.

By the way, why should generating a uuid ever fail? It's just a stinking random byte array. Fail to get MAC address? Fail to get timestamp? At this point I would rather have the application panic and dump a trace, because something is catastrophically wrong.

@kevinburke
Copy link
Contributor

In rare cases, you can't read random bytes from crypto/rand.Reader.

@mattwilliamson
Copy link

Retry or return nil? Backwards compatibility is important stuff. I don't know why you would downplay it.

@kevinburke
Copy link
Contributor

I'm not downplaying it. See earlier in the thread where I described how I maintain a fork now. You asked why it could return an error and I told you.

@domino14
Copy link

domino14 commented Apr 4, 2018

@satori pls

@ktnyt
Copy link

ktnyt commented Apr 5, 2018

It's been three months so I feel like this issue is pretty much, technically, closed in terms of discussion (unless we hear back from @satori ). No reversion. Deal with it, or resort to an alternative.

tl;dr

What happened?

Some old interfaces (uuid.NewV1, uuid.NewV2, and uuid.NewV4) were broken: wrap them with uuid.Must to achieve identical results (e.g. uuid.Must(uuid.NewV4())).

Alternatives?

If you want to use a pre-change compliant API go with kevinburke/go.uuid. google/uuid is another seemingly good alternative but beware as it publicly states that it may be unstable (although I doubt there being any huge API change).

@domino14
Copy link

domino14 commented Apr 5, 2018

That’s fine. We just want him to tag it so that we can use dep without overrides.

@theckman
Copy link

Some members of the Go community are now maintaining a fork of this repo. We've just cut a v2.0.0 which is for the breaking change. Our fork keeps v1.2.0 available, if you still need it:

kinbiko added a commit to bugsnag/bugsnag-go that referenced this issue Sep 7, 2018
satori/go.uuid introduced a breaking change a while back, which
can cause transient dependencies to be difficult to manage.

gofrs is an organisation of community members who work to ensure
the packages people depend on don't cause backwards breaking changes and
is therefore probably a better choice.

For more information:
satori/go.uuid#66
xizhibei added a commit to xizhibei/gocelery that referenced this issue Nov 1, 2018
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

No branches or pull requests