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
Dieterbe merged 6 commits intomasterfrom
fix-go-generate-directory-update-msgp-v1.1
Feb 11, 2019
Merged

Fix go generate qa check + update msgp to v1.1#1214
Dieterbe merged 6 commits intomasterfrom
fix-go-generate-directory-update-msgp-v1.1

Conversation

@Dieterbe
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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