-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
move coalesce out of bazelbuild image #6709
Conversation
/test pull-test-infra-bazel-canary |
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
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, krzyzacy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these OWNERS Files:
Approvers can indicate their approval by writing |
/hold cancel |
#2731 (comment) gives some flavor as to why I added I think trying to do the rename in the same PR I was testing bazel on kubernetes was causing me pain, since the PR hadn't yet been checked into t-i effecting the rename. I think I was also trying to follow the pattern of versioning dependencies with the image, similar to the way we use a precompiled In practice, I never followed up on using |
FWIW We should definitely bake it in an image in the shiny integrated
bootstrap future.
Since we run it from a scenario right now though it seemed better to just
handle it the same way we handle scenarios for the moment.
Side note: I've also confirmed in the kanarynetes PR that switching between
the images breaks caching presumably due to CC changes. Unifying to "base
image with logging requirements and docker voodoo / runner" and "layer on
top with tools versioned by kubernetes release" should help when we force
bazel to track system toolchain versions. 🤞
…On Wed, Feb 7, 2018, 22:04 Jeff Grafton ***@***.***> wrote:
#2731 (comment)
<#2731 (comment)>
gives some flavor as to why I added coalesce.py to the bazelbuild image.
I think trying to do the rename in the same PR I was testing bazel on
kubernetes was causing me pain, since the PR hadn't yet been checked into
t-i effecting the rename.
I think I was also trying to follow the pattern of versioning dependencies
with the image, similar to the way we use a precompiled kubetest, and
that potentially the version of coalesce.py at head would not be the
version we'd want.
In practice, I never followed up on using coalesce.py in the image, and
it hasn't seemed to cause us any real issues.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6709 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4Bq2pQRejLbkbVK7EkeeI0Va1w1Fccks5tSo50gaJpZM4R9pgJ>
.
|
it turns out even though we bake this script into the image we only use it via the local test infra clone, we can move this out to hack in preparation for consolidating the images.