Add an initial importer for Image Repositories #1303
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,11 @@ package controller | |
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/golang/glog" | ||
|
|
||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/util" | ||
|
|
||
| buildapi "github.com/openshift/origin/pkg/build/api" | ||
| buildclient "github.com/openshift/origin/pkg/build/client" | ||
| buildutil "github.com/openshift/origin/pkg/build/util" | ||
|
|
@@ -33,63 +35,62 @@ type ImageChangeController struct { | |
| } | ||
|
|
||
| // HandleImageRepo processes the next ImageRepository event. | ||
| func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageRepository) error { | ||
| glog.V(4).Infof("Build image change controller detected imagerepo change %s", repo.Status.DockerImageRepository) | ||
| subs := make(map[string]string) | ||
|
|
||
| // TODO: this is inefficient | ||
| for _, bc := range c.BuildConfigStore.List() { | ||
| config := bc.(*buildapi.BuildConfig) | ||
|
|
||
| shouldBuild := false | ||
| // For every ImageChange trigger find the latest tagged image from the image repository and replace that value | ||
| // throughout the build strategies. A new build is triggered only if the latest tagged image id or pull spec | ||
| // differs from the last triggered build recorded on the build config. | ||
| for _, trigger := range config.Triggers { | ||
| if trigger.Type != buildapi.ImageChangeBuildTriggerType { | ||
| continue | ||
| } | ||
| change := trigger.ImageChange | ||
|
There was a problem hiding this comment. -1, it's not a change, it's a trigger. specifically an image change trigger. There was a problem hiding this comment. Actually, it's the parameters to an image change trigger. It's the definition of what should change. ----- Original Message -----
There was a problem hiding this comment. the parameters would include the actual new imageid, this doesn't. this is the vessel, not the values. |
||
| // only trigger a build if this image repo matches the name and namespace of the ref in the build trigger | ||
| // also do not trigger if the imagerepo does not have a valid DockerImageRepository value for us to pull | ||
| // the image from | ||
| if repo.Status.DockerImageRepository == "" || change.From.Name != repo.Name || (len(change.From.Namespace) != 0 && change.From.Namespace != repo.Namespace) { | ||
|
There was a problem hiding this comment. -1 to removing code comments explaining the logical flow. There was a problem hiding this comment. I believe the comments were moved up (and reworded a bit) - see line 47. There was a problem hiding this comment. so they were. +1 to cancel my -1. :) |
||
| continue | ||
| } | ||
| latest, err := imageapi.LatestTaggedImage(repo, change.Tag) | ||
| if err != nil { | ||
| util.HandleError(fmt.Errorf("unable to find tagged image: %v", err)) | ||
| continue | ||
| } | ||
|
|
||
| // (must be different) to trigger a build | ||
| last := change.LastTriggeredImageID | ||
| next := latest.Image | ||
| if len(next) == 0 { | ||
| // tags without images should still trigger builds (when going from a pure tag to an image | ||
| // based tag, we should rebuild) | ||
| next = latest.DockerImageReference | ||
| } | ||
| if len(last) == 0 || next != last { | ||
| subs[change.Image] = latest.DockerImageReference | ||
|
There was a problem hiding this comment. DockerImageReference is a fully qualified/tagged/immutable value? There was a problem hiding this comment. Yes ----- Original Message -----
|
||
| change.LastTriggeredImageID = next | ||
|
There was a problem hiding this comment. shouldn't this be latest.DockerImageReference? There was a problem hiding this comment. No, see the comment that tags are not required to have images (we may not have a record of the image, but we may still have a pull spec) ----- Original Message -----
There was a problem hiding this comment. that's my point... what we should be recording here is the exact pull spec we used last time we built this, so we don't build it again using that same pull spec. i'm not clear on what "latest.Image" is (i haven't read this full PR) but it doesn't seem as uniquely identifying to a specific imageid/hash. There was a problem hiding this comment. If we're pulling from an external registry and it doesn't support pull by id (v2 registry or the tag/id hack we're doing for v1), then the best you can do is pull by tag. latest.Image is an actual image id There was a problem hiding this comment. ImageID is not guaranteed to be there. PullSpec is. If you transfer a tag from a pull spec to an image, or an image to a pull spec, then a change triggers the build. We need to store something. The field is poorly named. ----- Original Message -----
There was a problem hiding this comment. (and if there is a reason to run that build again, because latest.DockerImageReference does not uniquely identify a specific image hash and you want to pick up the new layer that latest.DockerImageReference now represents, then this change is breaking the fundamental premise of the image change controller which is that the build it produces can be rerun at any time and will use the exact same image layers) There was a problem hiding this comment. When we have virtual pull specs (tag points to id) the only thing that will change will be the id. But a build should be triggered anyway. ----- Original Message -----
There was a problem hiding this comment. Pull spec by tags will continue to be supported - we only guarantee a pull spec by image change will fire if the specs don't match, or the ids don't match. ----- Original Message -----
There was a problem hiding this comment. Sounds like this is going to need some work when this merges: There was a problem hiding this comment. Yes ----- Original Message -----
|
||
| shouldBuild = true | ||
| } | ||
| } | ||
|
|
||
| if shouldBuild { | ||
| glog.V(4).Infof("Running build for buildConfig %s in namespace %s", config.Name, config.Namespace) | ||
| b := buildutil.GenerateBuildFromConfig(config, nil, subs) | ||
| if err := c.BuildCreator.Create(config.Namespace, b); err != nil { | ||
| return fmt.Errorf("error starting build for buildConfig %s: %v", config.Name, err) | ||
| } | ||
| if err := c.BuildConfigUpdater.Update(config); err != nil { | ||
| // This is not a retryable error because the build has been created. The worst case | ||
| // outcome of not updating the buildconfig is that we might rerun a build for the | ||
| // same "new" imageid change in the future, which is better than running the build | ||
| // 2+ times by retrying it here. | ||
| return ImageChangeControllerFatalError{Reason: fmt.Sprintf("Error updating buildConfig %s with new LastTriggeredImageID", config.Name), Err: err} | ||
| } | ||
| } | ||
| } | ||
|
|
||
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.
-1