Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Fix go generate qa check + update msgp to v1.1 #1214

Merged
merged 6 commits into from
Feb 11, 2019

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Feb 4, 2019

We had newer generated code (with the new sentinel) but were vendoring the older library.
our qa check would compare against the older generated code, but didn't notice anything because it was executing go generate in the wrong dir.

Going over the changelog, seems mostly fixes and a few new features,
i don't see anything breaking or bleeding edge.
@replay
Copy link
Contributor

replay commented Feb 4, 2019

I'm not sure I understand the problem... When Gopkg.Toml specifies the version, and then we do dep ensure, this will put the correct specified version into the vendor dir, right? But then we also did go get in go-generate.sh which previously obtained the latest version, and that's the one we used to actually generate the library. Correct? And we didn't notice that before because go generate was executed in the wrong dir and didn't change anything.

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Feb 4, 2019

When Gopkg.Toml specifies the version, and then we do dep ensure, this will put the correct specified version into the vendor dir, right?

yes

But then we also did go get in go-generate.sh which previously obtained the latest version, and that's the one we used to actually generate the library. Correct?

no, it checks out a specific version to build the msgp tool, which generates our messagepack code (when we run go generate), and that generated code uses the msgp library that is vendored, and thus should be at the same version.

And we didn't notice that before because go generate was executed in the wrong dir and didn't change anything.

yes

@woodsaj
Copy link
Member

woodsaj commented Feb 4, 2019

could we update go-generate.sh to read the revision number directly from Gopkg.lock and checkout out that commit?

@robert-milan
Copy link
Contributor

I don't remember why right now, but there was a reason we didn't upgrade this last time. There is/was a breaking change when upgrading this library IIRC. I'll go try to find it.

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Feb 5, 2019

@woodsaj that's a great idea! pushing that now.
@robert-milan i was also wondering if there's a specific reason we haven't updated yet, i imagine there is one but i also couldn't find it.

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Feb 8, 2019

If no one can find the problem with the msgp library, can i get an approval on this?
Once I do, I'll update the schema dependency properly, which is currently causing the build failure.

@Dieterbe Dieterbe requested a review from woodsaj February 8, 2019 08:34
Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Dieterbe added a commit to raintank/schema that referenced this pull request Feb 11, 2019
@Dieterbe Dieterbe merged commit 544f48f into master Feb 11, 2019
@Dieterbe
Copy link
Contributor Author

for the record, deployed this to our internal ops env today using an in-place upgrade.
both during, as well as after the upgrade, everything stayed nominal.

@Dieterbe Dieterbe added this to the vnext milestone Feb 11, 2019
@Dieterbe Dieterbe deleted the fix-go-generate-directory-update-msgp-v1.1 branch March 27, 2019 21:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants