Allow builds to reference image repositories by name #679
Conversation
| // Registry is the Docker registry which should receive the resulting built image via push. | ||
| Registry string `json:"registry,omitempty" yaml:"registry,omitempty"` | ||
| // DockerImageReference is the full name of an image ([registry/]name[:tag]), and will be the | ||
| // value sent to Docker push at the end of a build. |
There was a problem hiding this comment.
"will be the value sent to Docker push at the end of the build if there is no To ImageRepo reference defined"
should also clarify in the comments how invalid/missing ImageRepo references are handled (fallback to the DockerImageReference value, fail the build, or do nothing?)
There was a problem hiding this comment.
you should indicate the "must set only one" condition for the dockerimagereference/to fields in the comments here, not just enforce it in the validation logic. Especially since the current comment says To takes precedence over DockerImageReference, but the validation enforces that only one can be set.
There was a problem hiding this comment.
Only validation on build config. Validation on builds does not. I'll add a comment on the build config parameters object.
----- Original Message -----
- // Registry is the Docker registry which should receive the resulting
built image via push.- Registry string
json:"registry,omitempty" yaml:"registry,omitempty"- // DockerImageReference is the full name of an image
([registry/]name[:tag]), and will be the- // value sent to Docker push at the end of a build.
you should indicate the "must set only one" condition for the
dockerimagereference/to fields in the comments here, not just enforce it in
the validation logic. Especially since the current comment says To takes
precedence over DockerImageReference, but the validation enforces that only
one can be set.
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/679/files#r23217305
| tag = fmt.Sprintf("%s/%s", build.Parameters.Output.Registry, tag) | ||
| } | ||
| return tag | ||
| return build.Parameters.Output.DockerImageReference |
There was a problem hiding this comment.
I don't see any value of leaving this method as is, I'd rather like to get rid of it.
smarterclayton
force-pushed
the
reference_build_output
branch
3 times, most recently
from
6f798d3
to
37d1b73
Compare
|
@ironcladlou @pmorie can one of you review from smarterclayton@8493302 onwards? |
smarterclayton
changed the title
| @@ -17,14 +17,14 @@ type CustomBuildStrategy struct { | |||
|
|
|||
| // CreateBuildPod creates the pod to be used for the Custom build | |||
| func (bs *CustomBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, error) { | |||
| buildJSON, err := json.Marshal(build) | |||
| data, err := v1beta1.Codec.Encode(build) | |||
There was a problem hiding this comment.
Inject the codec from the BuildControllerFactory through the typeBasedFactoryStrategy.
smarterclayton
force-pushed
the
reference_build_output
branch
2 times, most recently
from
ef05795
to
e9a54f4
Compare
|
I'd appreciate any review to be done by 10am EST tomorrow so I can try to land this. There's a data race in the integration tests that might be a bug in golang (if it continues i'll disable race testing on integration), otherwise this should be up to date with everything except Dan's overlapping changes. |
|
[test] |
Instead of using ImageTag and Registry, which must be joined together, allow clients to provide one of two options. "to", which may be a reference to an ImageRepository, or "dockerImageReference", which is a Docker push spec (the value you would tag an image before docker pushing it to a registry). Support backwards compatibility for ImageTag and Registry. Fix versioning of "BUILD" output - the field must be to a locked API version so that an upgrade during a build doesn't break it. Add proper validations on Build and BuildConfig, so that names, namespaces, and labels validate according to Kubernetes rules, and also increase the validation around the two new fields.
Also change builds to go into "Error" status when a problem occurs
and record a message. Slightly refactor the build controller code.
If the build pod already exists, assume that there is a race in build
execution and stay in "pending" state.
This means that clients that want to create ImageRepositories and
BuildConfigs at the same time that are using the integrated registry
only need to specify:
POST /imageRepositories {
"metadata": {
"name": "myimage",
},
}
POST /buildConfigs {
"parameters": {
"output": {
"to": {
"name": "myimage",
}
}
}
}
and the build will be configured with the right registry name automatically.
The integrated registry must be configured properly and OpenShift must be
configured to default to it via OPENSHIFT_DEFAULT_REGISTRY.
We want to phase out DockerImageRepository over time.
At runtime, dynamically load and cache the value of OPENSHIFT_DOCKER_REGISTRY
by allowing environment substitution that matches service behavior. The
registry must be in the default namespace and the default value is now:
OPENSHIFT_DOCKER_REGISTRY='${DOCKER_REGISTRY_SERVICE_HOST}:${DOCKER_REGISTRY_SERVICE_PORT}'
If the variables do not resolve (for instance, the service does not exist yet),
then the server will act as if there is no default registry and will return
empty string in Status.DockerImageRepository for ImageRepositories that don't
have the DockerImageRepository field set (meaning they don't "follow" a remote
repository).
Once the string is loaded it is remembered until restart, so deleting and
restarting the default 'docker-registry' service will result in the status
field being unavailable. In the future, OpenShift may create an empty
docker-registry service by default on startup that can then be matched
to real pods.
Preserve existing behavior, but ensure that From takes precendence.
Use testing.Logf instead of glog, add method to verbose log etcd, ensure as many tests as possible are stopping their servers.
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/802/) |
General cleanup of some code paths, add better error handling and document things that still need to change. Significant refactor to config_generator to avoid calling ListImageRepositories as much as possible. Use the new "from" object reference in preference to "repository" field. Deployment config logic is still a bit fragile - we need to come back and have the discussion on the proposed changes to behavior here later. Also, need more tests.
Use quotes everywhere, fix some Linux/Mac wierdnesses. Finally put all the e2e output under a single dir. Add `make release`
smarterclayton
force-pushed
the
reference_build_output
branch
from
e9a54f4
to
5193072
Compare
|
Given that this logic now allows use of a default registry, would it make sense to go ahead and auto-start said registry so we can just get rid of docker-registry-config.json altogether? |
|
Maybe - however we still need to have the config setup for secrets, @liggitt owns bringing that in. I think there's value in us having a simpler path, but at least right now the template needs to be able to make it work. Once we have that, we should consider the bootstrap. I also don't know if we want to take on "automatic" in the event we find people want to install their own. ----- Original Message -----
|
|
merging - will follow up with any review. [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/677/) (Image: devenv-fedora_584) |
|
Evaluated for origin up to b176c34 |
Merged by openshift-bot
…sersAdmission-allow-SA-with-implicit-NS Merged by openshift-bot
First two parts of a three part change (the third part is ensuring the default
registry is configured automatically).
Generalized cleanup of build code, including more validations, handling build
error states, and encoding build JSON into the build pod as our external (not
internal) schema.