Simplify godep licenses logic #26348
Conversation
thockin
added
the
release-note-none
k8s-github-robot
added
the
size/L
ghost
mentioned this pull request
ghost
added
priority/critical-urgent
| @@ -32,9 +32,9 @@ fi | |||
| # to work with docker-machine on macs | |||
| mkdir -p "${KUBE_ROOT}/_tmp" | |||
| _tmpdir="$(mktemp -d "${KUBE_ROOT}/_tmp/kube-godep-licenses.XXXXXX")" | |||
| echo "Created workspace: ${_tmpdir}" | |||
| #echo "Created workspace: ${_tmpdir}" | |||
There was a problem hiding this comment.
nit: Remove dead comments here and two lines down?
There was a problem hiding this comment.
I left them for the next poor debugger, but I could nix them.
thockin
force-pushed
the
fix-godep-licenses
branch
from
a8cca8d to
710ba0a
Compare
|
@thockin We need to go get and cache the missing license data from the internet. I don't know any other way around that. The old godep license process was several times slower than this and required hand-maintaining an exclusion list. The artifact from that was simply a list of licenses and this new version gathers actual license content dynamically across all packages and does it several times faster. I commented in the other issue. What is the repro case here for the failure? |
|
GCE e2e build/test passed for commit 710ba0a7adbdcb1bbc04350e70e1bda532b7f634. |
|
@david-mcmahon Hard to pin down. I spent hours trying to grok what was happening - back-to-back verify cycles would produce different results and then I caught it occasionally flaking in the (now removed) part. I decided it was wrong and excised it. |
|
@david-mcmahon FYI I ran it a few times on two different machines at different times, and it produced no output, or error message, ever. I didn't debug it any further than that. |
ghost
mentioned this pull request
|
OK, I tested this, and it seems to do what's required. Code looks fine to me. LGTM. |
ghost
added
the
lgtm
ghost
mentioned this pull request
|
Fixes #26328 |
alex-mohr
added
the
needs-rebase
|
@thockin It seems that there are some merge conflicts on this? |
|
@thockin It would be awesome if we could get this resolved ASAP, as there is a whole train of 1.3-blocking PR's queued up behind this one. |
This code used to actually reach out to the internet to look for files. This is flaky, slow, and semantically WRONG. The license that is upstream might actually be different than what we have vendored. Only look at local files. This now passes back-to-back updates and verifies.
thockin
force-pushed
the
fix-godep-licenses
branch
from
710ba0a to
847b56b
Compare
thockin
removed
the
needs-rebase
k8s-github-robot
removed
the
lgtm
thockin
added
the
lgtm
|
GCE e2e build/test passed for commit 847b56b. |
|
Manually merging to unblock dependent ubernetes PRs while submit queue is down. e2es passed, I assume this wouldn't have broken only unit tests. |
|
@quinton-hoole Writing "fixes #issue" in a PR comment doesn't close the issue. That needs to go in the PR description. Consequently, #26328 is still open. |
|
@bgrant0607 noted, thanks! |
|
@bgrant0607 FYI, according to this any commit message should do it, not only the PR description. So I still don't actually understand why my commit message above didn't work. |
|
@quinton-hoole Github comments aren't commit messages. |
This code used to actually reach out to the internet to look for files. This
is flaky, slow, and semantically WRONG. The license that is upstream might
actually be different than what we have vendored. Only look at local files.
This now passes back-to-back updates and verifies.
@quinton-hoole @mhrgoog @bgrant0607 - FYI
@zmerlynn @david-mcmahon - do you guys think this will satisfy legal? The older form was WAY complicated and a regular source of pain.