GoogleContainerTools
Go Upgrades
Description As discussed in office hours, draft PR showing go dependency upgrades.
- Many dependencies have published breaking changes so tests and fixes are needed in Skaffold. These are at the top of the stack.
- The commit containing the result of 'go mod vendor' is too large for me to push to Github. I hit HTTP 400 errors. This has been broken up into many smaller commits to fit under the github limits.
@renzodavid9 updated now that I figured out the github commit size issue.
This may be worth reviewing with your teammate regardless of the ssh issue, as updating dependencies is probably a good idea overall.
Do note the one fix that is in vendor/.
Hi @aran, I'm not sure what discussions you had with Renzo about this, and he's away for a bit, so I can't ask him at the moment... but this PR is 400,000 lines (!!). Even if you don't include the changes from go mod vendor, this is still a huge CL with a lot of unrelated changes.
I appreciate all the work you did fixing build breakages from the library upgrades, but it's going to be very difficult to review a PR this large. Could you maybe split it into a single PR for each dependency or something? Or at least a few at a time instead of literally all of them at once?
Thanks for your help with this!
I think skaffold's go deps have not been upgraded in a long time, so they pile up. I'm not sure what review tooling you use, but the way to look at this is at the commit level, not the overall PR. The git commit messages describe the commits as accurately as possible. I don't have a way to certify that the commits contain what the messages say they contain, so you may prefer to have a trusted maintainer re-do the commands.
There's basically a division of three conceptual sets of commits here.
1/ directly committing the results of 'go' commands, with no other changes:
-
go get -u ./... -
go mod tidy -
go mod vendor
These are labeled chore:. The issue is that the result of go mod vendor exceeds the maximum commit size allowed by github. I broke it up into dozens of smaller commits, partly by trial-and-error, to get each commit under github's limit. These are organized alphabetically by dep name.
Some of the skaffold code does not work with the newer version of the deps, so then there is
2/ fixes to skaffold to use the latest APIs from the updated Go deps.
These commits are labeled 'fix:', except that it looks like I missed one ("Create a logger for buildpacks build")
3/ The last commit is a fix to a vendored dep (buildpacks), which itself was out of date with the others. This would ideally be done upstream. I think it might be another Google project, so maybe you have resources I don't have to get that done upstream? Alternatively, the fix in the vendor folder is simple.
I don't have bandwidth or suitable tooling to automate breaking this down much further. To tell you the truth I don't really think the outsider PR is ideal for something like this. You can decide that you trust me and land it, which is probably the easiest path forward, but overall would be better for a maintainer to do this kind of dep upgrade work in a trusted manner, and in that case the section 2/ and 3/ commits may be a useful time-saver. Or of course skaffold's deps can be left out of date and it'll be a while before something breaks.
Thanks for the patient explanation, I really appreciate it. And yes, now I see how you helpfully broke all this down into individual steps, I didn't understand the process, but that makes a lot of sense. And, as is probably clear, I definitely didn't realize how vendored deps worked, now I understand why this is 400K lines 😁
Unfortunately, now I've gone and created a bunch of merge conflicts for you by merging some dependabot PRs.
To tell you the truth I don't really think the outsider PR is ideal for something like this. You can decide that you trust me and land it, which is probably the easiest path forward, but overall would be better for a maintainer to do this kind of dep upgrade work in a trusted manner, and in that case the section 2/ and 3/ commits may be a useful time-saver.
Yeah, I think that's a great point. I think I will go ahead and do a series of PRs to get all these dependencies updated. I'll base it heavily on your work here, so I really appreciate you taking the time to put this all together!
This may be worth reviewing with your teammate regardless of the ssh issue
I'm curious, what's this ssh issue?
@plumpy the ssh issue was https://github.com/GoogleContainerTools/skaffold/pull/9521 / https://github.com/GoogleContainerTools/skaffold/issues/9484, thank you for merging that PR!
I'll close this out since you have another plan now.