Removing hack/udpate-staging* from update-all.sh #44829
Conversation
rewording the failure message in verify-staging* scripts; adding instruction to hack/godep-restore.sh failure message
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caesarxuchao
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
size/S
|
This change is |
caesarxuchao
added
release-note-none
|
I think this is okay in the interim, but calls out the need for a better solution for go dependency management. cc: @kubernetes/sig-contributor-experience-misc |
| @@ -25,5 +25,9 @@ source "${KUBE_ROOT}/hack/lib/util.sh" | |||
| kube::util::ensure_godep_version v79 | |||
|
|
|||
| echo "Starting to download all kubernetes godeps. This takes a while" | |||
| GOPATH=${GOPATH}:${KUBE_ROOT}/staging godep restore "$@" | |||
| if ! GOPATH=${GOPATH}:${KUBE_ROOT}/staging godep restore "$@"; then | |||
| echo "\"godep restore\" failed. Removing the packages that caused the failure from your GOPATH might solve the problem." | |||
There was a problem hiding this comment.
The error messages of godep restore will indicate which packages cause the failure, so i think it's ok to refer them as "the packages" here.
There was a problem hiding this comment.
This isn't helpful. Like it or not, go encourages a shared GOPATH - I am not going to nuke my changes in other repos to satisfy this tool.
There was a problem hiding this comment.
If we EVER expect ANYONE outside of the people on this PR to run these scripts, they MUST be self-contained. We can make a temporary GOPATH and restore into that, for example.
There was a problem hiding this comment.
+1 for the temporary gopath in _output which is kept (until make clean).
There was a problem hiding this comment.
Caching in _output sounds promising. I don't have time to try it until later this week though.
|
This could also be an option but are we confident enough with godeps? I
have seen restore fail before even with a clean GOPATH and the fix was
..... to rerun it :)
…On Tue, Apr 25, 2017 at 10:29 AM, Dr. Stefan Schimanski < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In hack/godep-restore.sh
<#44829 (comment)>
:
> @@ -25,5 +25,9 @@ source "${KUBE_ROOT}/hack/lib/util.sh"
kube::util::ensure_godep_version v79
echo "Starting to download all kubernetes godeps. This takes a while"
-GOPATH=${GOPATH}:${KUBE_ROOT}/staging godep restore "$@"
+if ! GOPATH=${GOPATH}:${KUBE_ROOT}/staging godep restore "$@"; then
+ echo "\"godep restore\" failed. Removing the packages that caused the failure from your GOPATH might solve the problem."
+1 for the temporary gopath in _output which is kept (until make clean).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#44829 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADuFf2xavLtygnakxSl4oaThaa6vPov5ks5rza8CgaJpZM4NFmlw>
.
|
|
I have not seen it fail with a clean GOPATH
On Tue, Apr 25, 2017 at 2:16 AM, Michail Kargakis <notifications@github.com>
wrote:
… This could also be an option but are we confident enough with godeps? I
have seen restore fail before even with a clean GOPATH and the fix was
..... to rerun it :)
On Tue, Apr 25, 2017 at 10:29 AM, Dr. Stefan Schimanski <
***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In hack/godep-restore.sh
> <https://github.com/kubernetes/kubernetes/pull/
44829#discussion_r113134048>
> :
>
> > @@ -25,5 +25,9 @@ source "${KUBE_ROOT}/hack/lib/util.sh"
> kube::util::ensure_godep_version v79
>
> echo "Starting to download all kubernetes godeps. This takes a while"
> -GOPATH=${GOPATH}:${KUBE_ROOT}/staging godep restore "$@"
> +if ! GOPATH=${GOPATH}:${KUBE_ROOT}/staging godep restore "$@"; then
> + echo "\"godep restore\" failed. Removing the packages that caused the
failure from your GOPATH might solve the problem."
>
> +1 for the temporary gopath in _output which is kept (until make clean).
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/kubernetes/kubernetes/pull/
44829#discussion_r113134048>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/
ADuFf2xavLtygnakxSl4oaThaa6vPov5ks5rza8CgaJpZM4NFmlw>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#44829 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVFAHA1V8r0Zeucm03Ymvh7-Lwtcfks5rzbntgaJpZM4NFmlw>
.
|
|
How far back in time do you want to go? older versions of godep could fail on an empty gopath. And you can have a failure when github, or gopkg.in, or other servers go down. although with modern versions of godep I've never seen it fail on an emtpy gopath... |
|
As far as godep versioning goes, the hack/ scripts are currently pinned to v79 so the outputs should be predictable. |
k8s-github-robot
added
the
needs-rebase
|
@caesarxuchao PR needs rebase |
|
This PR hasn't been active in 36 days. It will be closed in 53 days (Jul 24, 2017). You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days |
|
update-staging-client.sh will run much faster after #44784, because it doesn't run godep anymore. I'll look into optimizing update-staging-godeps.sh later. |
Addressing #36770 (comment)
More discussion at #44519 (comment)
A summary of the discussion:
godep restoredoesn't work reliably when the GOPATH is not clean. The two update-staging* scripts rely ongodep restore, thus causing troubles to developers. This PR removes the two scripts from update-all.sh, but keeps the coresonding verification in the verify-all.sh, so developers only have to run the update-staging* scripts when necessary.cc @thockin @Kargakis