Please tag a new release after #49

[creachadair]

#49 adds basic support for Go modules, but the latest current release tag (v1.0.0) is dated March 2017. Could you please find a suitable point at or after #49 to add a newer tag? (I think v1.1.0 is a reasonable choice, since there don't seem to be any API changes, but I didn't check exhaustively).

[bradfitz]

Hey @jba, is apidiff ready for use?

[jba]

It runs, and it works. But it's not stable and may never be in its currrent form—it will probably get rolled into go release, or become an API.

So definitely suitable to run manually as part of an informal or occasional process, but I wouldn't bake it into my CI just yet.

[creachadair]

In the meantime, would you be willing to tag a minor version on something at or after #49?

If your tool is somewhere accessible, I'd be willing to have a Go [sic] at checking its results.

[bradfitz]

@creachadair, could you try it? https://godoc.org/golang.org/x/exp/cmd/apidiff

[creachadair]

Sure! It looks pretty safe to me. Here's a typescript of what I did (after building the tool):

$ git clone git@github.com:ghodss/yaml.git
$ cd yaml
$ git checkout -b test v1.0.0
$ apidiff -w old github.com/ghodss/yaml
$ rm go.mod go.sum
$ git checkout master
$ apidiff -w new github.com/ghodss/yaml
$ apidiff old new
Incompatible changes:
- Unmarshal: changed from func([]byte, interface{}) error to func([]byte, interface{}, ...JSONOpt) error
Compatible changes:
- DisallowUnknownFields: added
- JSONOpt: added
- UnmarshalStrict: added
- YAMLToJSONStrict: added

While strictly the addition of the options to the type signature is a breaking change, in practice it should be safe because no pre-existing call site will have any options there.

Of course if someone passed the function as an argument, it could still break, but I expect that to be rare.

[creachadair]

If you would like to avoid any breaking changes, I would be game to rework the interface to add UnmarshalWithOptions and make Unmarshal use it.

[bradfitz]

That'd be the responsible thing, except that would probably break even more people who are already using Unmarshal with options in the ~year since 8aa0426.

We could do a v2 I guess. But that might warrant other cleanups. And it means changing import paths on everybody.

Thoughts?

[creachadair]

That'd be the responsible thing, except that would probably break even more people who are already using Unmarshal with options in the ~year since 8aa0426.

Given that there is only one option defined by the package (viz., DisallowUnknownFields), I don't think it's necessarily that serious a problem. I assume some people probably are using that, though.

We could do a v2 I guess. But that might warrant other cleanups. And it means changing import paths on everybody.

Thoughts?

I'm sympathetic to the desire to avoid breaking changes. Ordinarily I'd say anybody pinned to a random hash at head kind of takes their chances—but historically that was what go get does by default, so I can't really fault people for it. I've run into this several times now, where the migration to modules gets hung up on the lack of dependency hygiene in the pre-modules world. I'd like to do the right thing, but I have higher priorities.

It's kind of a pity one can't ship a go fix plugin—if we could I'd be tempted to just do that, since the expected usage shouldn't be that bad to patch. Otherwise, I suppose a v2 is probably the safe alternative. That wouldn't require fixing anybody till they opt in.

[creachadair]

Re: Cutting a v2. This seems to me like a reasonable path forward, given the API change (even though I suspect the impact would be minimal). You mentioned "other cleanups", do you have anything specific in mind?

[bradfitz]

I hadn't even looked, but a v2 seems like a good time to do cleanups.

[creachadair]

I hadn't even looked, but a v2 seems like a good time to do cleanups.

That certainly makes sense, but what I'm looking for here is a way to make progress toward having a tagged release newer than March 2017, and it's not my project. I'm happy to help make it better along the way, but would prefer not to wind up owning it.

[creachadair]

Another datum: The Go wiki recommends incrementing the major version when switching to modules as as a "best practice", even if there are no other changes (since, as @bcmills points out, a change to the import path is also a change to the API).

Since the API is already changed from the last tag, our options seem to be to (a) leave things as they are now (viz., a bitrotten v1.0.0 from two years ago), (b) get the new version out there with the existing API changes, or (c) identify some specific goals for cleanup.

I suppose there are also options (d) remove the unmaintained dependency and work around it; or (e) fork it and maintain it myself. I'd rather help move this one along than do either of those, but I'd prefer (d) to (a).

[bcmills]

The Go wiki recommends incrementing the major version when switching to modules as as a "best practice"

Note that that advice applies only to modules already at major version 2 or higher, since those versions will likely switch from lacking a semantic-version path suffix to having one. Adding a go.mod file at v1.1.0 should be fine.

[creachadair]

Note that that advice applies only to modules already at major version 2 or higher, since those versions will likely switch from lacking a semantic-version path suffix to having one. Adding a go.mod file at v1.1.0 should be fine.

Right, agreed: but in this case we already have a breaking API change that just hasn't been tagged yet.

[bcmills]

In that case, it seems like the damage is already done: there are likely consumers of the package — without a /v2 suffix — both before and after v1.0.0, so you pretty much have to decide which of them to break either way.

If you're going to break the post-v1.0.0 users, then you may as well roll back the breaking API change, and otherwise you may as well bite the bullet and break the v1.0.0 users with the v1.1.0 tag.

[creachadair]

In that case, it seems like the damage is already done: there are likely consumers of the package — without a /v2 suffix — both before and after v1.0.0, so you pretty much have to decide which of them to break either way.

Adding a v2 should not break any existing consumer, as each dependent can choose whether to update to the new major version in their own time. My suggestion is to leave the existing v1 code as-is. I initially proposed a v1.1.0 tag, but in light of the subsequent discussion (above) it seems clear that's not safe. That's actually why I think incrementing the major version makes sense here.

[jimdickinson]

Is anything likely to change w/ this issue? I imagine a lot of people hit this problem where a very old ghodss/yaml version is picked up by go mod tidy when switching over to using go mod.

[creachadair]

Is anything likely to change w/ this issue? I imagine a lot of people hit this problem where a very old ghodss/yaml version is picked up by go mod tidy when switching over to using go mod.

I'm still willing to contribute to cleanup in support of tagging a v2. I also think it would be fine to tag it as v2 as it stands, and I believe that would be preferable to the current status. However, as you can see from the thread above, there does not appear to be much interest from the owners of the package.

That leaves, as far as I can tell, three options: (1) Stop using it, (2) Fork it, or (3) Live with it. So far I have been doing (3), but I think (2) might be easier than (1) given the APi differences from https://godoc.org/gopkg.in/yaml.v2. One way or another I would like to break dependencies on packages that are not being maintained.

[osdrv]

For what it's worth, there are quite some massive projects that tie their ghodss/yaml dependency to revision c7ce166, e.g.:

• https://github.com/kubernetes/apiserver/blob/master/Godeps/Godeps.json#L102
• https://github.com/kubernetes/apiextensions-apiserver/blob/master/Godeps/Godeps.json#L126
• https://github.com/helm/helm/blob/master/glide.lock#L72 (implicit, but still resolves to this version)

I wonder if it makes sense to tag c7ce166 as 1.1.0?

[creachadair]

I wouldn't object to that, but I believe c7ce166 already subsumes the addition of the options to the Unmarshal signature, which is what makes the API change breaking as noted in #50 (comment) et seq.

[osdrv]

@creachadair thanks a lot for your reply. Indeed, there is a little chance the change is in fact breaking. A few hours after I posted my question, I discovered that k8s sig folks forked the library and tagged it around the aforementioned commit (including the breaking changes to Unmarshal) as 1.1.0 (https://github.com/kubernetes-sigs/yaml/tree/v1.1.0). Not sure I'm making any real point by bringing this fact up, but some projects started migrating to sigs.k8s.io/yaml, e.g.: https://github.com/containerd/containerd/blob/master/vendor.conf#L77

[creachadair]

Not sure I'm making any real point by bringing this fact up, but some projects started migrating to sigs.k8s.io/yaml, e.g.: https://github.com/containerd/containerd/blob/master/vendor.conf#L77

That's a useful data point, thanks. If the Kubernetes folks are on-board to keep maintaining that package as a reusable module, I might look into switching kythe.io to use their fork instead.