s2i: s2i rebuild broken #570
Conversation
| return nil, err | ||
| } | ||
|
|
||
| r, err := PullImage(config.Tag, d, api.PullNever, false) |
There was a problem hiding this comment.
why not pull if not present? they could have pushed it somewhere.
There was a problem hiding this comment.
i was making the assumption that it was always local ... if we can't make that assumption, do we repurpose the builder pull policy or previous image pull policy, or do we have to add another field in the config for rebuild pull policy ?
There was a problem hiding this comment.
and until the deprecated field is remove, i can leverage the ForcePull field as well
There was a problem hiding this comment.
thinking about it some more, I'll reuse the builder policy ... the tester for example specified the -p option ... seems natural
|
@gabemontero Looks like it was broken long time ago or it's not? |
|
@php-coder yep, as I mentioned in the bugzilla, no recent changes jumped out as something that would have broke |
gabemontero
force-pushed
the
bug1366475
branch
from
4de98f1
to
adcdd6a
Compare
|
responses to comments pushed |
| @@ -97,7 +98,8 @@ func describeBuilderImage(config *api.Config, image string, out io.Writer) { | |||
| Tag: config.Tag, | |||
| IncrementalAuthentication: config.IncrementalAuthentication, | |||
| } | |||
| build.GenerateConfigFromLabels(c) | |||
| pr, _ := docker.GetBuilderImage(c) | |||
There was a problem hiding this comment.
why aren't you checking this error?
There was a problem hiding this comment.
The error was ignored before. Assumption I took was to describe as much as
possible. I was inclined to not change that.
On Friday, August 12, 2016, Ben Parees notifications@github.com wrote:
In pkg/api/describe/describer.go
#570 (comment)
:@@ -97,7 +98,8 @@ func describeBuilderImage(config *api.Config, image string, out io.Writer) {
Tag: config.Tag,
IncrementalAuthentication: config.IncrementalAuthentication,
}
- build.GenerateConfigFromLabels(c)
- pr, _ := docker.GetBuilderImage(c)
why aren't you checking this error?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/source-to-image/pull/570/files/adcdd6aa20b77e9c9ec5dce3b524cf20b22042ca#r74624778,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADbadIK2fgbY9YEDd5Fg96RVaYb9zLaqks5qfKcIgaJpZM4JjLrA
.
There was a problem hiding this comment.
but now pr could be nil, in which case GenerateConfigFromLabels is going to error anyway, seems better to report the error as early as possible, otherwise it's going to get wrapped/lost (before the error from the pull got bubbled up as is)
There was a problem hiding this comment.
OK ... pushed update that does not ignore the err
On Fri, Aug 12, 2016 at 1:49 PM, Ben Parees notifications@github.com
wrote:
In pkg/api/describe/describer.go
#570 (comment)
:@@ -97,7 +98,8 @@ func describeBuilderImage(config *api.Config, image string, out io.Writer) {
Tag: config.Tag,
IncrementalAuthentication: config.IncrementalAuthentication,
}
- build.GenerateConfigFromLabels(c)
- pr, _ := docker.GetBuilderImage(c)
but now pr could be nil, in which case GenerateConfigFromLabels is going
to error anyway, seems better to report the error as early as possible,
otherwise it's going to get wrapped/lost (before the error from the pull
got bubbled up as is)—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/source-to-image/pull/570/files/adcdd6aa20b77e9c9ec5dce3b524cf20b22042ca#r74631929,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADbadDMZo_UdRKnFwa6WQRAsw1MDL31Rks5qfLIdgaJpZM4JjLrA
.
|
LGTM. Thank you! |
Bug 1366475 Bugzilla link https://bugzilla.redhat.com/show_bug.cgi?id=1366475 Detailed explanation an attempt to pull the builder image to obtain labels was made; the correct thing was to pull the locally build image we were rebuilding
gabemontero
force-pushed
the
bug1366475
branch
from
adcdd6a
to
8c24edb
Compare
|
lgtm |
|
Source To Image Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_sti/189/) |
|
Evaluated for source to image merge up to 8c24edb |
Bug 1366475
Bugzilla link https://bugzilla.redhat.com/show_bug.cgi?id=1366475
Detailed explanation
an attempt to pull the builder image to obtain labels was made; the correct thing was
to pull the locally build image we were rebuilding
@bparees @php-coder PTAL