build trigger for upstream image changes #557

bparees · 2014-12-10T21:25:26Z

No description provided.

ncdc · 2014-12-10T21:26:45Z

pkg/build/api/types.go

@@ -37,6 +37,9 @@ type BuildParameters struct {

 	// Output describes the Docker image the Strategy should produce.
 	Output BuildOutput `json:"output,omitempty" yaml:"output,omitempty"`
+
+	// UpstreamImage indicates the upstream docker image that should be used for this build
+	UpstreamImage string `json:"dependentImage,omitempty" yaml:"dependentImage,omitempty"`


json/yaml mapping name should be consistent with field name, shouldn't it?

bparees · 2014-12-10T21:27:17Z

@marianitadn @soltysh fyi. Maciej, I don't think you've started the image change trigger bit, so i'm going to move on to that as i realize now adding the upstream image field didn't really represent much effort other than naming :)

maria · 2014-12-11T10:39:33Z

pkg/build/controller/image_change_controller.go

+	Stop <-chan struct{}
+}
+
+type icBuildConfigInterface interface {


what ic stands for?

i copied this from the deploymentcontroller but i believe it stands for imageChange.

bparees · 2014-12-15T04:41:35Z

This code is currently a non-compiling mess, but I wanted to put it in here so @soltysh doesn't overlap the work i've done to implement the image_change_controller so far.

Should also note that this implementation so far appears to be at odds with what @smarterclayton has in mind for this feature, so that still needs to be discussed before carrying this much further.

bparees · 2014-12-22T21:40:20Z

@soltysh @jhadvig ok this is in much better shape now...it actually works, at least for STI builders (haven't tested for docker yet).

things not yet done:

implement substitution for custombuilds (probably will search all env var declarations for matching image names to replace)
checking on startup that we have run a build for the current imageid associated with the imagerepository for all image change triggers (in case a new image was created while the controller was down).
tests

If you're looking for something to do, either #2 or #3 are reasonable things to look at..but let me know which, if either, you are doing so i can focus my efforts on the other parts.

Note that if you're playing with this, you need to push the ruby-20-centos image into the local docker-registry, which is why i've updated the template json for the moment.

smarterclayton · 2014-12-22T21:52:21Z

----- Original Message -----

@soltysh @jhadvig ok this is in much better shape now...it actually works, at
least for STI builders (haven't tested for docker yet).

things not yet done:

implement substitution for custombuilds (probably will search all env var
declarations for matching image names to replace)

This may need to be opt-in - how do you know for sure that the image name should be specific (i.e. what if the custom build wants to use that value to perform some logic of its own against docker)?

checking on startup that we have run a build for the current imageid
associated with the imagerepository for all image change triggers (in case a
new image was created while the controller was down).

tests

If you're looking for something to do, either #2 or #3 are reasonable things
to look at..but let me know which, if either, you are doing so i can focus
my efforts on the other parts.

Note that if you're playing with this, you need to push the ruby-20-centos
image into the local docker-registry, which is why i've updated the template
json for the moment.

Reply to this email directly or view it on GitHub:
#557 (comment)

bparees · 2014-12-22T21:58:47Z

@smarterclayton agreed, how we address custombuild strategies is not fully thought through yet. We could add an explicit "baseimage" field like we have for docker-builds, and put the impetus on the custombuilder to use that field if desired, but i don't love that idea since there's no particular requirement a custombuilder even uses a baseimage, so it becomes an (optional) parameter that doesn't necessarily make any sense.

We could also only substitute specific keys but that's not much better than adding the optional first-class field, if it's better at all.

do you see a third alternative?

smarterclayton · 2014-12-22T22:05:33Z

We've talked about a more sophisticated ENV story than what we have now, where you could indicate via env other things beyond just "use this value", similar to generation. There's nothing upstream yet so I'm hesitant to invent something. For step one, we can simply omit it from custom build and require you to use the ENV var we set as part of the base build.

----- Original Message -----

@smarterclayton agreed, how we address custombuild strategies is not fully
thought through yet. We could add an explicit "baseimage" field like we
have for docker-builds, and put the impetus on the custombuilder to use that
field if desired, but i don't love that idea since there's no particular
requirement a custombuilder even uses a baseimage, so it becomes an
(optional) parameter that doesn't necessarily make any sense.

We could also only substitute specific keys but that's not much better than
adding the optional first-class field, if it's better at all.

do you see a third alternative?

Reply to this email directly or view it on GitHub:
#557 (comment)

bparees · 2014-12-22T22:12:01Z

if we do that of course we need to pick an env var key that will not collide with anything they might want to use.

smarterclayton · 2014-12-22T22:14:01Z

Possibly reuse the same one as the base docker build - if they want to override it they can, if they don't override it they get the value we set.

On Dec 22, 2014, at 5:12 PM, Ben Parees notifications@github.com wrote:

if we do that of course we need to pick an env var key that will not collide with anything they might want to use.

—
Reply to this email directly or view it on GitHub.

soltysh · 2015-01-09T13:30:16Z

test/integration/imagechange_buildtrigger_test.go

+func TestSimpleImageChangeBuildTrigger(t *testing.T) {
+	t.Log("starting run")
+	deleteAllEtcdKeys()
+	//openshift := NewTestOpenshift(t)


I'm pretty confident you can nuke this comment 😉

bparees · 2015-01-09T20:15:37Z

@soltysh comments mostly addressed aside from the ones i commented on.

openshift-bot · 2015-01-12T05:14:28Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/711/)

bparees · 2015-01-12T19:10:47Z

@smarterclayton can I get a final review on this? The one big hole i'm aware of is that builds triggered by means other than an image change, will simply use the imageid from the original buildconfig rather than getting the latest imageid from the imagerepo(if one is associated with the build).

i'm not even sure that's a huge problem (since if you setup your buildconfig+image tags correctly, your other builds will get the right thing anyway), but I think we can loop back and fix it later if it is important.

smarterclayton · 2015-01-12T19:24:08Z

pkg/build/api/types.go

+	// performing a custom build, if needed.
+	CustomBuildStrategyBaseImageKey = "OPENSHIFT_CUSTOM_BUILD_BASE_IMAGE"
+
+	// DefaultImageTag is used when an image tag is needed and the configuration


This comment is incomplete

comment completed

smarterclayton · 2015-01-12T20:56:35Z

pkg/build/util/generate.go

+			}
+		}
+		// update the actual custom build image with the new image, if applicable
+		if build.Parameters.Strategy.CustomStrategy.Image == oldImage {


doh. fixed.

bparees · 2015-01-12T22:19:01Z

pkg/build/api/validation/validation.go

@@ -100,7 +100,10 @@ func validateStrategy(strategy *buildapi.BuildStrategy) errs.ValidationErrorList
 			allErrs = append(allErrs, validateSTIStrategy(strategy.STIStrategy).Prefix("stiStrategy")...)
 		}
 	case buildapi.DockerBuildStrategyType:
-		// DockerStrategy is currently optional
+		// DockerStrategy is currently optional, initialize it to a default state if it's not set.


@smarterclayton does this address your validation concern?

Note that the other types don't even test for nil

STI does. Custom does not and should. i'll add that one.

smarterclayton · 2015-01-13T02:27:28Z

LGTM - ready for merge?

bparees · 2015-01-13T02:54:03Z

Quite. [merge]

openshift-bot · 2015-01-13T02:56:23Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/592/) (Image: devenv-fedora_531)

openshift-bot · 2015-01-13T02:56:24Z

Evaluated for origin up to 17d0023

Merged by openshift-bot

OSEv3.5.0.5

@krancour

* Passing a RESTClient through to the TPR storage cc/ @krancour * fixing more compile errs * Fixing test compile error * fixing integration test compile err * removing redundant import * disabling TPR integration tests enabling them will be done in a follow-up

bparees force-pushed the add_dependency_id branch from d68ec8c to 07ed8ab Compare December 10, 2014 21:25

bparees changed the title ~~add an upstream image field to builds~~ [WIP][DO_NOT_MERGE] add an upstream image field to builds Dec 10, 2014

ncdc reviewed Dec 10, 2014
View reviewed changes

bparees force-pushed the add_dependency_id branch 2 times, most recently from 499b9a3 to 3101a9f Compare December 10, 2014 21:59

maria reviewed Dec 11, 2014
View reviewed changes

bparees force-pushed the add_dependency_id branch from 3101a9f to 89d462c Compare December 15, 2014 04:39

bparees force-pushed the add_dependency_id branch 6 times, most recently from f054e2c to 2de71ff Compare December 22, 2014 17:02

bparees mentioned this pull request Dec 22, 2014

[WIP] Add base image from the build UpstreamImage if set #561

Closed

bparees force-pushed the add_dependency_id branch 2 times, most recently from 4d7b5be to 8f2fe96 Compare December 22, 2014 21:36

bparees force-pushed the add_dependency_id branch from 8f2fe96 to e8da2e3 Compare December 22, 2014 21:44

soltysh reviewed Jan 9, 2015
View reviewed changes

bparees force-pushed the add_dependency_id branch from e187f66 to 28b6c35 Compare January 9, 2015 20:58

bparees force-pushed the add_dependency_id branch from 286d8c6 to 8646c39 Compare January 12, 2015 19:08

bparees changed the title ~~[DO_NOT_MERGE] build trigger for upstream image changes~~ build trigger for upstream image changes Jan 12, 2015

smarterclayton reviewed Jan 12, 2015
View reviewed changes

bparees force-pushed the add_dependency_id branch from 8646c39 to 41e4a8b Compare January 12, 2015 20:08

smarterclayton reviewed Jan 12, 2015
View reviewed changes

bparees force-pushed the add_dependency_id branch 2 times, most recently from 345ae7f to 275caa2 Compare January 12, 2015 22:18

bparees reviewed Jan 12, 2015
View reviewed changes

introduce image change build trigger logic

17d0023

bparees force-pushed the add_dependency_id branch from 275caa2 to 17d0023 Compare January 12, 2015 22:42

openshift-bot pushed a commit that referenced this pull request Jan 13, 2015

Merge pull request #557 from bparees/add_dependency_id

a5189c7

Merged by openshift-bot

openshift-bot merged commit a5189c7 into openshift:master Jan 13, 2015

bparees deleted the add_dependency_id branch January 16, 2015 21:10

sjenning pushed a commit to sjenning/origin that referenced this pull request Jan 24, 2017

Merge pull request openshift#557 from tdawson/OSEv3.5.0.5

573a59b

OSEv3.5.0.5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build trigger for upstream image changes #557

build trigger for upstream image changes #557

bparees commented Dec 10, 2014

ncdc Dec 10, 2014

bparees Dec 10, 2014

bparees commented Dec 10, 2014

maria Dec 11, 2014

bparees Dec 15, 2014

bparees commented Dec 15, 2014

bparees commented Dec 22, 2014

smarterclayton commented Dec 22, 2014

bparees commented Dec 22, 2014

smarterclayton commented Dec 22, 2014

bparees commented Dec 22, 2014

smarterclayton commented Dec 22, 2014

soltysh Jan 9, 2015

bparees commented Jan 9, 2015

openshift-bot commented Jan 12, 2015

bparees commented Jan 12, 2015

smarterclayton Jan 12, 2015

bparees Jan 12, 2015

smarterclayton Jan 12, 2015

bparees Jan 12, 2015

bparees Jan 12, 2015

smarterclayton Jan 12, 2015

smarterclayton Jan 12, 2015

bparees Jan 12, 2015

bparees Jan 12, 2015

smarterclayton commented Jan 13, 2015

bparees commented Jan 13, 2015

openshift-bot commented Jan 13, 2015

openshift-bot commented Jan 13, 2015

build trigger for upstream image changes #557

build trigger for upstream image changes #557

Conversation

bparees commented Dec 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Dec 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Dec 15, 2014

bparees commented Dec 22, 2014

smarterclayton commented Dec 22, 2014

bparees commented Dec 22, 2014

smarterclayton commented Dec 22, 2014

bparees commented Dec 22, 2014

smarterclayton commented Dec 22, 2014

Choose a reason for hiding this comment

bparees commented Jan 9, 2015

openshift-bot commented Jan 12, 2015

bparees commented Jan 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jan 13, 2015

bparees commented Jan 13, 2015

openshift-bot commented Jan 13, 2015

openshift-bot commented Jan 13, 2015