build trigger for upstream image changes #557
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,23 +44,23 @@ type BuildStatus string | |
|
|
||
| // Valid values for BuildStatus. | ||
| const ( | ||
| // BuildStatusNew is automatically assigned to a newly created build. | ||
| BuildStatusNew BuildStatus = "New" | ||
|
|
||
| // BuildStatusPending indicates that a pod name has been assigned and a build is | ||
| // about to start running. | ||
| BuildStatusPending BuildStatus = "Pending" | ||
|
|
||
| // BuildStatusRunning indicates that a pod has been created and a build is running. | ||
| BuildStatusRunning BuildStatus = "Running" | ||
|
|
||
| BuildStatusComplete BuildStatus = "Complete" | ||
| // BuildStatusComplete indicates that a build has been successful. | ||
|
|
||
| // BuildStatusFailed indicates that a build has executed and failed. | ||
| BuildStatusFailed BuildStatus = "Failed" | ||
|
|
||
| // BuildStatusError indicates that an error prevented the build from executing. | ||
| BuildStatusError BuildStatus = "Error" | ||
|
|
||
| // BuildStatusCancelled indicates that a running/pending build was stopped from executing. | ||
|
|
@@ -72,7 +72,7 @@ type BuildSourceType string | |
|
|
||
| // Valid values for BuildSourceType. | ||
| const ( | ||
| //BuildSourceGit is a Git SCM | ||
| BuildSourceGit BuildSourceType = "Git" | ||
| ) | ||
|
|
||
|
|
@@ -175,6 +175,11 @@ type DockerBuildStrategy struct { | |
| // NoCache if set to true indicates that the docker build must be executed with the | ||
| // --no-cache=true flag | ||
| NoCache bool `json:"noCache,omitempty" yaml:"noCache,omitempty"` | ||
|
|
||
| // BaseImage is optional and indicates the image that the dockerfile for this | ||
| // build should "FROM". If present, the build process will substitute this value | ||
| // into the FROM line of the dockerfile. | ||
| BaseImage string `json:"baseImage,omitempty" yaml:"baseImage,omitempty"` | ||
| } | ||
|
|
||
| // STIBuildStrategy defines input parameters specific to an STI build. | ||
|
|
@@ -226,6 +231,20 @@ type WebHookTrigger struct { | |
| Secret string `json:"secret,omitempty" yaml:"secret,omitempty"` | ||
| } | ||
|
|
||
| // ImageChangeTrigger allows builds to be triggered when an ImageRepository changes | ||
| type ImageChangeTrigger struct { | ||
| // Image is used to specify the value in the BuildConfig to replace with the | ||
| // immutable image id supplied by the ImageRepository when this trigger fires. | ||
| Image string `json:"image" yaml:"image"` | ||
| // ImageRepositoryRef a reference to a Docker image repository to watch for changes. | ||
| ImageRepositoryRef *kapi.ObjectReference `json:"imageRepositoryRef" yaml:"imageRepositoryRef"` | ||
| // Tag is the name of an image repository tag to watch for changes. | ||
|
Contributor
There was a problem hiding this comment. Specify what the behavior is if no tag is specified.
Contributor
Author
There was a problem hiding this comment. I need to update this, none of these fields are going to be optional.
Contributor
There was a problem hiding this comment. If tag isn't optional, we should make it not optional on all APIs. Or is tag not optional in the other places its used? ----- Original Message -----
Contributor
Author
There was a problem hiding this comment. I haven't looked at all the other use cases, but in this use case, the only way we can make it non-optional is if we default to "latest". i'm fine with doing that if you prefer it, but there's no way to perform image change trigger logic without some tag value to be watching for changes.
Contributor
There was a problem hiding this comment. Right, if deployment is defaulting "" to "latest" we should too. I think there's value in not requiring people to specify it, and some cost for clients to have to deal with it. If you don't default, everyone will pick their own (Git i.e. uses branch master by default, and origin as the remote by default, so everyone deals with it). ----- Original Message -----
|
||
| Tag string `json:"tag,omitempty" yaml:"tag,omitempty"` | ||
| // LastTriggeredImageID is used internally by the ImageChangeController to save last | ||
| // used image ID for build | ||
| LastTriggeredImageID string `json:"lastTriggeredImageID,omitempty" yaml:"lastTriggeredImageID,omitempty"` | ||
| } | ||
|
|
||
| // BuildTriggerPolicy describes a policy for a single trigger that results in a new Build. | ||
| type BuildTriggerPolicy struct { | ||
| // Type is the type of build trigger | ||
|
|
@@ -236,6 +255,9 @@ type BuildTriggerPolicy struct { | |
|
|
||
| // GenericWebHook contains the parameters for a Generic webhook type of trigger | ||
| GenericWebHook *WebHookTrigger `json:"generic,omitempty" yaml:"generic,omitempty"` | ||
|
|
||
| // ImageChange contains parameters for an ImageChange type of trigger | ||
| ImageChange *ImageChangeTrigger `json:"imageChange,omitempty" yaml:"imageChange,omitempty"` | ||
| } | ||
|
|
||
| // BuildTriggerType refers to a specific BuildTriggerPolicy implementation. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,11 +100,18 @@ func validateStrategy(strategy *buildapi.BuildStrategy) errs.ValidationErrorList | |
| allErrs = append(allErrs, validateSTIStrategy(strategy.STIStrategy).Prefix("stiStrategy")...) | ||
| } | ||
| case buildapi.DockerBuildStrategyType: | ||
| // DockerStrategy is currently optional, initialize it to a default state if it's not set. | ||
|
Contributor
Author
There was a problem hiding this comment. @smarterclayton does this address your validation concern?
Contributor
There was a problem hiding this comment. Note that the other types don't even test for nil
Contributor
Author
There was a problem hiding this comment. STI does. Custom does not and should. i'll add that one. |
||
| if strategy.DockerStrategy == nil { | ||
| strategy.DockerStrategy = &buildapi.DockerBuildStrategy{} | ||
| } | ||
| case buildapi.CustomBuildStrategyType: | ||
| if strategy.CustomStrategy == nil { | ||
| allErrs = append(allErrs, errs.NewFieldRequired("customStrategy", strategy.CustomStrategy)) | ||
| } else { | ||
| // CustomBuildStrategy requires 'image' to be specified in JSON | ||
| if len(strategy.CustomStrategy.Image) == 0 { | ||
| allErrs = append(allErrs, errs.NewFieldRequired("image", strategy.CustomStrategy.Image)) | ||
| } | ||
| } | ||
| default: | ||
| allErrs = append(allErrs, errs.NewFieldInvalid("type", strategy.Type, "type is not in the enumerated list")) | ||
|
|
@@ -138,25 +145,31 @@ func validateTrigger(trigger *buildapi.BuildTriggerPolicy) errs.ValidationErrorL | |
|
|
||
| // Ensure that only parameters for the trigger's type are present | ||
| triggerPresence := map[buildapi.BuildTriggerType]bool{ | ||
| buildapi.GithubWebHookBuildTriggerType: trigger.GithubWebHook != nil, | ||
| buildapi.GenericWebHookBuildTriggerType: trigger.GenericWebHook != nil, | ||
| } | ||
| allErrs = append(allErrs, validateTriggerPresence(triggerPresence, trigger.Type)...) | ||
|
|
||
| // Validate each trigger type | ||
| switch trigger.Type { | ||
| case buildapi.GithubWebHookBuildTriggerType: | ||
| if trigger.GithubWebHook == nil { | ||
| allErrs = append(allErrs, errs.NewFieldRequired("github", nil)) | ||
| } else { | ||
| allErrs = append(allErrs, validateWebHook(trigger.GithubWebHook).Prefix("github")...) | ||
| } | ||
| case buildapi.GenericWebHookBuildTriggerType: | ||
| if trigger.GenericWebHook == nil { | ||
| allErrs = append(allErrs, errs.NewFieldRequired("generic", nil)) | ||
| } else { | ||
| allErrs = append(allErrs, validateWebHook(trigger.GenericWebHook).Prefix("generic")...) | ||
| } | ||
| case buildapi.ImageChangeBuildTriggerType: | ||
| if trigger.ImageChange == nil { | ||
| allErrs = append(allErrs, errs.NewFieldRequired("imageChange", nil)) | ||
| } else { | ||
| allErrs = append(allErrs, validateImageChange(trigger.ImageChange).Prefix("imageChange")...) | ||
| } | ||
| } | ||
| return allErrs | ||
| } | ||
|
|
@@ -171,6 +184,21 @@ func validateTriggerPresence(params map[buildapi.BuildTriggerType]bool, t builda | |
| return allErrs | ||
| } | ||
|
|
||
| func validateImageChange(imageChange *buildapi.ImageChangeTrigger) errs.ValidationErrorList { | ||
| allErrs := errs.ValidationErrorList{} | ||
| if len(imageChange.Image) == 0 { | ||
| allErrs = append(allErrs, errs.NewFieldRequired("image", "")) | ||
| } | ||
| if imageChange.ImageRepositoryRef == nil { | ||
| allErrs = append(allErrs, errs.NewFieldRequired("imageRepositoryRef", "")) | ||
| } else if len(imageChange.ImageRepositoryRef.Name) == 0 { | ||
| nestedErrs := errs.ValidationErrorList{errs.NewFieldRequired("name", "")} | ||
| nestedErrs.Prefix("imageRepositoryRef") | ||
| allErrs = append(allErrs, nestedErrs...) | ||
| } | ||
| return allErrs | ||
| } | ||
|
|
||
| func validateWebHook(webHook *buildapi.WebHookTrigger) errs.ValidationErrorList { | ||
| allErrs := errs.ValidationErrorList{} | ||
| if len(webHook.Secret) == 0 { | ||
|
|
||
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.
We should stick with proper naming, so id -> name, please.
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.
are you referring to the RepositoryName field? i'm maintaining consistency w/ deployments:
https://github.com/openshift/origin/blob/master/pkg/deploy/api/types.go#L153
however the entire field is going to change to be an ObjectRef shortly so i'll consider a name change to just "repository" at that point.
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.
You have this correct here - refs to foo should be "FooRef" as the field name