incident.ID or incident.Id

[mblaschke]

In incident.go the Incident struct defines the Id and from the APIObject also the field ID which both gets parsed from json:"id".

Sometimes ID is filled sometimes Id.

webdevops/pagerduty-exporter#8

[stmcallister]

Oh, the joys of inheriting a library. :) Thanks for pointing this out! Were you ever able to get a value back for incident.ID? I'm unable to do so. And, technically, it should be incident.APIObject.ID. But, still no value there for me either.

[mblaschke]

If the Id field from the incident struct is removed the issue should be fixed :)

[rtfb]

If the Id field from the incident struct is removed the issue should be fixed :)

Yes, but removal of Id would be a breaking change. I suppose the only fix for this issue is to find all the places where Incident is populated and cross-populate the fields.

(And then deprecate Id as a clear signal that it was a blooper)

[mblaschke]

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?
This bug is now opened since April and I had to add a detection which field is used in all apps. Not really happy with that.

[mblaschke]

any progress here?

[mblaschke]

Still no progress here?

[mblaschke]

Ping?

[theckman]

No progress to report. Thank you for checking in.

[theckman]

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.

[theckman]

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.

[mblaschke]

I would more or less just remove the Id from the incident struct:
https://github.com/PagerDuty/go-pagerduty/blob/master/incident.go#L86

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 :)

[theckman]

@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?

[bcmills]

(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 Id field, removing its json: field tag, and adding a custom MarshalJSON method to the Incident struct. The MarshalJSON method could error out if the Id and ID fields have conflicting values, and otherwise marshal whichever one is non-empty.

Then the only programs that would break are those that unmarshal or write into the ID field and also explicitly modify the deprecated Id field. That is presumably a strict subset of the programs that would break if the Id field were removed entirely, and that subset of programs is likely almost entirely programs that are already broken because of the nondeterministic unmarshaling.

(Even if you decide to go the route of a full-on breaking change, you might consider the MarshalJSON option as a smoother migration path to give consumers time to migrate.)