move to maintained fork of mapstructure

[jm96441n]

Description

the mitchelh/matpstructure repo is being archived (see https://gist.github.com/mitchellh/90029601268e59a29e64e55bab1c5bdc) the go-viper fork is the maintained one https://github.com/go-viper/mapstructure?tab=readme-ov-file#migrating-from-githubcommitchellhmapstructure there should be no code changes required just changing the dependency

Testing & Reproduction steps

Links

PR Checklist

• updated test coverage
• external facing docs updated
• appropriate backport labels added
• not a security concern

[jm96441n]

so there was a small change in how errors are reported that we needed to update here

[zalimeni]

Generally LGTM, though noting this bit from the migration guide:

replace github.com/mitchellh/mapstructure => github.com/go-viper/mapstructure v1.6.0

it seems we could possibly make a smaller change to start, and ensure stability for backport patches until we're sure the change is baked, since as you noted this library is pervasive in its use. No strong concern moving to v2 on main given the only breaking change seems to be error formatting, but it's notable that not many other HC repos have migrated from <= 1.5.0, so if we don't have a strong need to jump now, incremental might be best.

[jmurret]

🙏

[jm96441n]

@zalimeni that's a good point, I jumped to v2 because of this line from the docs:

You can migrate to this package by changing your import paths in your Go files to github.com/go-viper/mapstructure/v2. The API is the same, so you don't need to change anything else.

I'm down for either, doing the replace is probably safer short term, but given the current state that v2 is meant to be compatible with v1 of the mitchelh and it would require any consumers of the api package to also include that replace I went for the v2 jump

[zalimeni]

and it would require any consumers of the api package to also include that replace I went for the v2 jump

That's a good callout. I think we might be able to chalk that up to the downstream's responsibility, since AFAIK there's no CVE currently pushing migration. If we get to that point, v2 backports probably becomes a necessity.

I'm also happy deferring to quorum since I see @jmurret approved and I think the risk is relatively low!

[jm96441n]

yeah, if we go down the replace route we'll need to update our usage docs for using the api package to ensure end users include that replace statement in their go.mod

[zalimeni]

yeah, if we go down the replace route we'll need to update our usage docs for using the api package to ensure end users include that replace statement in their go.mod

Yeah, diverging might not be ideal in that sense. Another option, and maybe a good starting point, would be to actually use that dependency (as you've done) rather than a replace; I'm not sure we have to replace to accomplish this. My main thought was more that pinning the 1.5.0 SHA from the fork, or using 1.6.0, is possibly safer than jumping at once to v2.

[jmurret]

I am in favor this and not using replace:

My main thought was more that pinning the 1.5.0 SHA from the fork, or using 1.6.0, is possibly safer than jumping at once to v2.

I kind of rubber stamped this and Michael gave a little more thought.

[jmurret]

request that we pin to 1.5 SHA or 1.6 without replace

[jm96441n]

can do, let me use the 1.6.0 release from the fork this isn't actually possible

[jm96441n]

so I'm not sure we can pin without a replace, our options are:

1. move to the v2 that is in the go-viper repository (which has kept the same api surface but used the v2 version to indicate a new module)
2. use the 1.6.0 version from the new repository but that would require a replace statement because the 1.6.0 version still declares itself as github.com/mitchellh/mapstructure https://github.com/go-viper/mapstructure/blob/8110e7183d594164869d292abf37d2b33611da5a/go.mod#L1
3. do nothing for now and leave it
   @jmurret @zalimeni

[zalimeni]

2. use the 1.6.0 version from the new repository but that would require a replace statement because the 1.6.0 version still declares itself as github.com/mitchellh/mapstructure https://github.com/go-viper/mapstructure/blob/8110e7183d594164869d292abf37d2b33611da5a/go.mod#L1

🤦🏻‍♂️ I missed this, sorry @jm96441n . Looks like that shuts down a simple migration path for submodule consumers until the jump.

I think of the remaining options, 1 or 3 makes the most sense. The risk of jumping to v2 seems low-but-not-0 given minor breaking run-time changes, but - given we aren't being forced by any CVE or incompatibility - I'd still be in favor of only doing so on main rather than backporting it, just out of caution (I'm also not trying to make a ⛰️ out of a molehill, and am happy to disagree and commit if others feel differently).

It's a great call for 1.21 LTS, but doesn't seem necessary for released versions, and we can always backport later if we need to. I'm also fine w/ 3 but seems this is a "when" not "if" problem, and we should definitely do it before 1.21, IMO.

[jm96441n]

no backporting makes sense to me! this was something I wanted to get ahead of before it actually became a problem given the original repos are going to be archived