-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
migrate to go modules from vndr #2941
Conversation
c240ddc
to
9eaa6c8
Compare
Codecov Report
@@ Coverage Diff @@
## master #2941 +/- ##
=======================================
Coverage 60.47% 60.47%
=======================================
Files 102 102
Lines 7999 7999
=======================================
Hits 4837 4837
Misses 2515 2515
Partials 647 647
Continue to review full report at Codecov.
|
@thaJeztah Thanks for the review. As suggested by you, I can do this in a follow-up PR. I wanted to scope this PR to just migration from |
No, definitely should try to do those in follow-ups and keep the diff in this one as minimal as possible (same version of dependencies) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The real question is how do we get people at Docker to look at these PRs. |
It's been created a day ago; it will be looked at |
I created a simple PR months ago, it's not been looked at. My confidence is low. |
@caervs requesting your review on this. |
e0fa8a3
to
198d97f
Compare
@caervs I've addressed the review comments. |
/cc @ehazlett for review (as you were working on something similar for containerd 🤗 ) |
Friendly ping for review. |
@tariq1890 sorry I should've made this a separate comment for easier visibility. Please address #2941 (comment) |
Overall LGTM |
@caervs I tried this locally, but I don't see how this is diffing and erroring out. It is possible that I may have missed some steps. Also, the steps I have written was taken from kube-state-metrics (a repository I maintain) and it does work . You can see go mod validate script we use - https://github.com/kubernetes/kube-state-metrics/blob/master/Makefile#L22 See this for details - https://travis-ci.org/kubernetes/kube-state-metrics/jobs/547266596. It is a build that was kicked off a couple of hours back. |
@tariq1890 the case that we care about here is that someone submits a PR that adds a new dependency, but forgets to vendor in the code for that library. The vendoring script here would pull in the new dep but would exit cleanly, so from the PR CI job we won't know that the vendor directory is actually out of sync. To verify this you can remove an existing library, commit, and then run the vendoring script
However if you add in
The last command will exit status |
Signed-off-by: Tariq Ibrahim <tariq181290@gmail.com>
Makes sense. Thanks for the clarification @caervs . |
You can also see examples of how we do vendor checking in the Prometheus common Makefile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. LGTM.
It's a petty go modules
pulls in so much more than vndr
did. Hopefully it's on the go team's agenda to reduce to just dependencies.
This PR should migrate docker/distribution to
go modules
.