Report multiple build causes for image change triggers #14777
Conversation
| Message: buildapi.BuildTriggerCauseImageMsg, | ||
| ImageChangeBuild: &buildapi.ImageChangeCause{ | ||
| ImageID: id, | ||
| ImageID: latest, |
There was a problem hiding this comment.
This might actually be a bug - I don't know that we depend on this, but it was not set before ever.
There was a problem hiding this comment.
yeah I don't think we depend on it, just informational to the user (we use the TriggeryByImage content to actually set the base image used for the build) but good catch regardless.
smarterclayton
force-pushed
the
improve_tests_for_buildconfigtrigger
branch
from
28a0f8a to
4133754
Compare
There was a problem hiding this comment.
i'm not convinced this new behavior makes sense. ultimately what this does is submit a buildrequest for a build with a single "triggeredbyimage" value. And the build that gets run will use that "triggeredbyimage" value as the base image, and update that specific trigger within the BC to indicate what image it last triggered on.
If 3 images change simultaneously, and the build has triggers for all 3, then we will go through 3 distinct buildrequests, each one containing one of the images as the "triggeredbyimage" value.
So it doesn't seem correct for any of those to also report all 3 images as the "buildtriggercause", because we're really only reacting to(running a build that explicitly consumes) a specific one of them.
it is true that if the BC consumes multiple images, we'll also be resolving the other imagestream references, but that's implicit, not explicit.
| Message: buildapi.BuildTriggerCauseImageMsg, | ||
| ImageChangeBuild: &buildapi.ImageChangeCause{ | ||
| ImageID: id, | ||
| ImageID: latest, |
There was a problem hiding this comment.
yeah I don't think we depend on it, just informational to the user (we use the TriggeryByImage content to actually set the base image used for the build) but good catch regardless.
|
|
||
| // prevent duplicate build trigger causes | ||
| if fired == nil { | ||
| fired = make(map[kapi.ObjectReference]string) |
There was a problem hiding this comment.
why not initialize this where you declared the variable? going for efficiency?
Um... why would we do that? |
|
We currently resolve all triggers at build instantiate time - so we would not trigger 3. |
ok yeah, i forgot we do that, however it's still the case that what is happening is we are running a build that explicitly picks up a particular image, and then implicitly picks up the rest. Meaning that the build is guaranteed to use the one image listed in the triggeredbyimage section (and what was supposed to be listed in the triggeredby section), it is not guaranteed to use all N images that you are now listing in the triggeredby section. Essentially you're just using the triggeredby section as "other stuff that was also different when we ran this build. the build may pick these up, if they are not replaced with something even newer before this build runs". If you prefer that, ok, it's certainly potentially useful information, but the implication/meaning is definitely a bit fuzzy. |
|
(well, "before this build runs" is an overstatement since the resolution happens during instantiate, not when the build runs. so there's almost no chance the build runs with anything different than what you're going to report, but it is non-zero) |
|
I think from an end user perspective, id want to see all of the causes listed.
|
then i guess my only open question is the way you're initializing the map, but i assume you're doing that to save initializing it when we don't enter the condition. |
|
Right, and to avoid doing a second resolution on an image and getting
a different one.
|
|
[test] |
|
Any other comments? |
|
lgtm |
|
[merge] |
|
[severity:bug] |
|
re[test] |
|
[merge]
…On Mon, Jun 26, 2017 at 7:07 PM, OpenShift Bot ***@***.***> wrote:
continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/merge_pull_
request_origin/1140/) (Base Commit: 496129f
<496129f>)
(PR Branch Commit: 4133754
<4133754>)
(Extended Tests: bug)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14777 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pxOwkBIBuHPtdf4rUf_mWPHnEyXiks5sIDm4gaJpZM4N_w8w>
.
|
|
continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 2 |
smarterclayton
force-pushed
the
improve_tests_for_buildconfigtrigger
branch
from
4133754 to
19872e4
Compare
|
Evaluated for origin merge up to 19872e4 |
|
[test] |
2 similar comments
|
[test] |
|
[test] |
|
Evaluated for origin test up to 19872e4 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2768/) (Base Commit: 478cfe2) (PR Branch Commit: 19872e4) |
Improve tests
[test]
@bparees this is not a fix to #14725, but increases coverage and is more accurate.