-
Notifications
You must be signed in to change notification settings - Fork 240
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
incident.ID or incident.Id #218
Comments
see PagerDuty/go-pagerduty#218 see #8 Signed-off-by: Markus Blaschke <[email protected]>
Oh, the joys of inheriting a library. :) Thanks for pointing this out! Were you ever able to get a value back for |
If the Id field from the incident struct is removed the issue should be fixed :) |
see PagerDuty/go-pagerduty#218 Signed-off-by: Markus Blaschke <[email protected]>
Yes, but removal of (And then deprecate |
Now it can happen that it is pure luck which field is used so isn't it better to introduce a breaking change then having a random issue in the code? |
any progress here? |
Still no progress here? |
Ping? |
No progress to report. Thank you for checking in. |
One potential path forward would be to present a PR and we can discuss the implementation there. Honestly speaking, I am trying to do everything I can to avoid releasing a v2 of this library because of Go Modules and the challenges it would introduce to consumers of this library. I am not sure how we could reliably know which ID field to use with the JSON parsing. I wonder if we should just release a breaking 1.x change and apologize profusely. This project was incepted before Modules and now we're kinda screwed. |
The bigger elephant in the room is that I think this library should probably be rewritten and released under a new name, partially to avoid the Modules challenges but to also give it a fresh start. To clarify, it never had a 0.x release, and so the API was never explored. We committed to a stable API at launch. Modules makes it really hard to upgrade major versions, and so we are totally hosed and have no way to explore the API now without massive hurdles. So I think we need a new project. Sadly, I am an ex-employee who happened to introduce Go at PagerDuty. I didn't incept this library itself, but I do feel a bit of a personal responsibility to shepherding it along. That said, I am not in a position to start a new repo (no access). I am also not sure the appetite for PagerDuty's side. |
I would more or less just remove the Current state: marshal/unmarshal can lead to unpredictable behaviour. Even if there is no version system other projects are still pinning the library to the commit hash (like any other package system). If people always use latest, sorry.. their fault. It's more or less like running docker images on latest tag. Luckily Golang is not PHP where you would not notice this change if you update the dependency to a new commit and try to compile it. People will notice the change as it just breaks the build. So they have to adapt their code when they update the dependency. But isn't that the reason why we pin the dependencies to a commit? :) IMHO: A bug is a bug and should be fixed.. especially when it triggers an unpredictable behaviour :) |
@mblaschke You've convinced me. The API compatibility guarantee should be made on stable APIs. Based on the nature of this bug I would argue this API is currently not stable, and thus should not be held to the same compatibility guarantee. We should avoid making breaking API changes, but for this bug don't think we can and I think that's okay. We should just be transparent and try to make it as painless as possible. @stmcallister I know you're OOO; when you're back could we sync-up quickly to try and align on a path forward for this issue? |
(I arrived here via golang/go#44550.) Is there a reason this has to be a breaking change at all? It seems like this could be addressed by instead deprecating the Then the only programs that would break are those that unmarshal or write into the (Even if you decide to go the route of a full-on breaking change, you might consider the |
In
incident.go
the Incident struct defines theId
and from theAPIObject
also the fieldID
which both gets parsed fromjson:"id"
.Sometimes
ID
is filled sometimesId
.webdevops/pagerduty-exporter#8
The text was updated successfully, but these errors were encountered: