Bump major version to v29 #1344
Conversation
|
Note to self: If this gets merged, then a release of this repo should be tagged with |
|
having a separate go.mod for the scrape package was very intentional (see #1308 (comment)). I likely won't be able to fully review this today, but do please wait to submit, as I'd like very much to keep the modules separate if at all possible. /cc @dmitshur for comment and review as well |
|
what if you just leave the scrape package as relying on v28, and don't update that one in this change? Then, after this PR has landed and the new tag created, update the scrape package to use v29 in a separate change. Would that work? |
|
Ah! OK. Sorry about that. I will fix it tonight and let you know when it is ready for review. |
|
Hi @willnorris - I believe this preserves the scrape versioning. (I was a bit surprised that the dependencies of scrape are now being brought up into the top-level PTAL. |
|
I agree with Will’s comments. We must not remove the scrape/go.mod file as that would change module boundaries. It’s a good idea to keep this PR focused on just one module; updating the scrape module can be more safely done in later PRs. Trying to make changes to more than one module (especially ones that are interconnected) in one PR is tricky and not worth doing until feeling very confident about the process. Glenn, have you done |
|
Thank you, @dmitshur! Now I've run 'go mod tidy'. 😄 PTAL. |
go.mod
Outdated
| github.com/google/go-querystring v1.0.0 | ||
| golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2 | ||
| golang.org/x/net v0.0.0-20190311183353-d8887717615a // indirect | ||
| golang.org/x/net v0.0.0-20191014212845-da9a3fd4c582 // indirect |
There was a problem hiding this comment.
This change doesn’t look intentional.
I suspect it came about from the earlier version of this PR where the scrape/go.mod file was removed, and its requirements merged with that of the main go-github module. The scrape module happened to require a newer version of x/net:
Line 10 in 04a52c2
And so it stayed around after go mod tidy because this module also needs x/net.
It’s not harmful to keep it since it’s just a newer x/net, but it’s also probably better to make changes intentionally rather than due to chance.
You can go back to original or stay with the newer, I’m okay with both.
There was a problem hiding this comment.
Ah, I see. Thank you, @dmitshur!
Reverted.
|
We discussed this PR in #1365. I would like to get approval from @willnorris on this PR since this is the first one containing Thank you! |
|
Thank you, @willnorris ! |
|
Ugh... I broke the build. Trying to figure out how to roll this back. |
|
#1367 should fix this release. |
This reverts commit cfb91a4.
Note to @willnorris - the only way I could figure out to get this to work (now with the inclusion of the scrape package) was to remove the
go.modandgo.sumfrom thescrapedirectory. (Because v29 ofgo-githubdoes not exist until the PR is committed and the repo is tagged.)I'm thinking this makes the most sense anyway since the top-level
go.modcovers the whole repo.If you find a better mechanism, though, I'm totally happy to close this PR and go with the better solution.