Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:You can indicate your approval by writing |
|
|
||
| endif # end of docker block that's restricted to Linux | ||
| # if first part of URL (i.e., hostname) is gcr.io then upload istioctl and deb | ||
| $(if $(findstring gcr.io,$(firstword $(subst /, ,$(HUB)))),$(eval push: gcs.push.istioctl-all gcs.push.deb),) |
There was a problem hiding this comment.
why is the upload conditional ? i'm a bit confused by this
There was a problem hiding this comment.
This is based on the original bin/push script, which had something like:
# Permit $HUB overrides to suppress errors during push
if [[ "${HUB}" =~ ^gcr\.io ]]; then
pilot/bin/upload-istioctl -p "gs://istio-artifacts/pilot/${TAG}/artifacts/istioctl" &
pilot/bin/push-debian.sh -c opt -p "gs://istio-artifacts/pilot/${TAG}/artifacts/debs" &
security/bin/push-debian.sh -c opt -p "gs://istio-artifacts/auth/${TAG}/artifacts/debs" &
fi
The rule is trying to do the make equivalent, whereby a "push" to GCS is done if the hub starts with gcr.io. I'm not a fan of the behavior in the original script (nor porting it to make) -- it seems like too much of a kludge to assume that a push to GCR also means they want a copy to GCS. I hesitantly ported the functionality to make so that my deletion of bin/push would be less controversial (by retaining feature-parity, though at the time I did the port I didn't know who/what needed the functionality and only the "upload-istioctl" line still existed [the debian lines were presumably removed as part of the consolidation to one debian file]).
My preference would be to have users do an explicit copy for themselves, though I don't quite know what that should look like. Offhand I'm thinking that someone would do a "make push", then a "make artifacts" (or maybe "artifacts" would already be a superset of "push"), and then manually copy the artifacts as needed, e.g.:
make push
make artifacts
gsutil -m cp -r "$(make where-is-out)"/istioctl-* "gs://istio-artifacts/pilot/${TAG}/artifacts/debs"
gsutil -m cp -r "$(make where-is-out)"/*.deb "gs://istio-artifacts/pilot/${TAG}/artifacts/debs"
and then none of the logic to detect when to do a copy (or defaulting of parameters like "GS_BUCKET") would be needed in the Makefile.
This description isn't necessarily an argument against the change (I did after all suggest it as a way to unblock testing), but I did want to present the background so other people can offer thoughts/opinions on which direction (or offer a third) on if/how the copying of artifacts to GCS should be handled.
|
@chxchx PR needs rebase |
|
|
||
| endif # end of docker block that's restricted to Linux | ||
| # if first part of URL (i.e., hostname) is gcr.io then upload istioctl and deb | ||
| $(if $(findstring gcr.io,$(firstword $(subst /, ,$(HUB)))),$(eval push: gcs.push.istioctl-all gcs.push.deb),) |
There was a problem hiding this comment.
This is based on the original bin/push script, which had something like:
# Permit $HUB overrides to suppress errors during push
if [[ "${HUB}" =~ ^gcr\.io ]]; then
pilot/bin/upload-istioctl -p "gs://istio-artifacts/pilot/${TAG}/artifacts/istioctl" &
pilot/bin/push-debian.sh -c opt -p "gs://istio-artifacts/pilot/${TAG}/artifacts/debs" &
security/bin/push-debian.sh -c opt -p "gs://istio-artifacts/auth/${TAG}/artifacts/debs" &
fi
The rule is trying to do the make equivalent, whereby a "push" to GCS is done if the hub starts with gcr.io. I'm not a fan of the behavior in the original script (nor porting it to make) -- it seems like too much of a kludge to assume that a push to GCR also means they want a copy to GCS. I hesitantly ported the functionality to make so that my deletion of bin/push would be less controversial (by retaining feature-parity, though at the time I did the port I didn't know who/what needed the functionality and only the "upload-istioctl" line still existed [the debian lines were presumably removed as part of the consolidation to one debian file]).
My preference would be to have users do an explicit copy for themselves, though I don't quite know what that should look like. Offhand I'm thinking that someone would do a "make push", then a "make artifacts" (or maybe "artifacts" would already be a superset of "push"), and then manually copy the artifacts as needed, e.g.:
make push
make artifacts
gsutil -m cp -r "$(make where-is-out)"/istioctl-* "gs://istio-artifacts/pilot/${TAG}/artifacts/debs"
gsutil -m cp -r "$(make where-is-out)"/*.deb "gs://istio-artifacts/pilot/${TAG}/artifacts/debs"
and then none of the logic to detect when to do a copy (or defaulting of parameters like "GS_BUCKET") would be needed in the Makefile.
This description isn't necessarily an argument against the change (I did after all suggest it as a way to unblock testing), but I did want to present the background so other people can offer thoughts/opinions on which direction (or offer a third) on if/how the copying of artifacts to GCS should be handled.
| gsutil -m cp -r "${ISTIO_OUT}"/istioctl-* "gs://${GS_BUCKET}/pilot/${TAG}/artifacts/istioctl" | ||
|
|
||
| gcs.push.deb: deb | ||
| gsutil -m cp -r "${ISTIO_OUT}"/*.deb "gs://${GS_BUCKET}/pilot/${TAG}/artifacts/debs" |
There was a problem hiding this comment.
Given my prior comment, I now see that the original bin/push script placed the debian files under different directories ("pilot" vs "auth"). On the one hand it seems inappropriate to place the monolithic debian under "pilot", but on the other hand both copies are placing the artifacts in sibling directories and there's a certain appeal to having the binaries close to each other. Maybe neither should be using "pilot", but I don't know how many other things would also need to be updated to handle the rename. Anyway, I don't have a particular update in mind but maybe others have ideas on what, if anything, should be done.
|
Based on the test failures it looks like fpm will need to be added to the prow docker image. Ideally this would also be added to the istio-builder image. These lines in release/cloud_build.template.json could be updated to the new image: "gcr.io/istio-testing/istio-builder:0.4.5-bazel7-go19", and then these lines in release/cloud_builder.sh could also be removed: apt-get -qqy install ruby ruby-dev rubygems build-essential gem install --no-ri --no-rdoc fpm I'd have a preference to still keep bazel in the images. It's still needed by the proxy repo and as recently as last night I was experimenting with trying to hack up an istio release build mechanism that can be used to consume a locally-compiled proxy. |
Automatic merge from submit-queue. Install fpm in prowbazel and istio-builder images Trying to fix istio/istio#3279 which eventually will fix istio/istio#2444
|
@chxchx: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Automatic merge from submit-queue. Upgrade istio-builder image to 0.4.6 See discussion in #3279 /assgin @mattdelco @sebastienvas @rkpagadala
Automatic merge from submit-queue. Install fpm in prowbazel and istio-builder images Trying to fix istio/istio#3279 which eventually will fix istio/istio#2444
[solo-io/istio merge job] Nightly merge of upstream into master-solo
Trying to get the test result