Convert builder images to go #347
Conversation
csrwng
force-pushed
the
go_build_scripts_2
branch
3 times, most recently
from
4aa473b
to
e9cd82d
Compare
| } | ||
|
|
||
| // dockerBuild performs a docker build on the source that has been retrieved | ||
| func (d *DockerBuilder) dockerBuild() (err error) { |
There was a problem hiding this comment.
In general when you have single return values, don't use named parameters. Named return values should be used judiciously.
csrwng
force-pushed
the
go_build_scripts_2
branch
2 times, most recently
from
e13a2f7
to
b9a7659
Compare
|
@smarterclayton - who do we need to sign off on this one to get it merged? |
|
I'm in the process of reviewing now |
|
|
||
| // DockerBuilder builds Docker images given a git repository URL | ||
| type DockerBuilder struct { | ||
| *builder |
There was a problem hiding this comment.
Generally embedding a pointer to a struct for anything except trivial wrapping is frowned on. Why can't this be an interface, and why does it have to be nested like this?
There was a problem hiding this comment.
The pattern was to share common code between DockerBuilder and STIBuilder, but I'll extract it out into its own interface.
csrwng
force-pushed
the
go_build_scripts_2
branch
from
b9a7659
to
33a1544
Compare
|
@soltysh thx, I think that's what we are doing now. |
|
@smarterclayton - I've pushed a new commit to address your concerns |
csrwng
force-pushed
the
go_build_scripts_2
branch
2 times, most recently
from
042f9c1
to
e9c044c
Compare
csrwng
force-pushed
the
go_build_scripts_2
branch
from
e9c044c
to
822b2d6
Compare
|
Made additional fixes suggested by @mfojtik |
| if _, err = builder.Build(); err != nil { | ||
| return err | ||
| } | ||
| if s.build.Parameters.Output.Registry != "" || s.authPresent { |
There was a problem hiding this comment.
Can't I push to the docker hub?
There was a problem hiding this comment.
Yes you can, only if an auth entry is present. Otherwise, we'll try to push only if you've specified a registry.
|
LGTM [merge], may have some follow up comments |
|
Origin Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/338/) (Image: devenv-fedora_353) |
|
[Test]ing while waiting on the merge queue |
|
Origin Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/395/) |
|
[test] |
|
What was the failure? Flakiness? |
|
It got further this time, still taking a look... the earlier failure was definitely flakiness, but this one might be a real issue. |
|
Yup, so the problem now is that we take the commit id from the webhook seriously (meaning we try to build that commit id) and the one in the end-to-end test is fake. Therefore the build fails. For this PR I'll just make that commit id blank. |
1 similar comment
|
Yup, so the problem now is that we take the commit id from the webhook seriously (meaning we try to build that commit id) and the one in the end-to-end test is fake. Therefore the build fails. For this PR I'll just make that commit id blank. |
csrwng
force-pushed
the
go_build_scripts_2
branch
from
822b2d6
to
1af5874
Compare
|
Updated the example github webhook json and separated the tag from the repository in the push image call. However, there still seems to be an issue with pushing the image: Testing again [test] |
|
So it looks like the registry only remains up for about 4 mins. At that point it dies and a new one is launched. If I run the e2e tests locally, everything works. But on the Jenkins box, it seems that the 4 min is not enough for the build to push the resulting image. |
|
The reason the registry keeps relaunching is because a new deployment is getting triggered periodically. The reason for the deployment is 'ConfigChange'. However the config doesn't seem to change from one deployment to the next. This is one of the deployments: {
"apiVersion": "v1beta1",
"controllerTemplate": {
"podTemplate": {
"desiredState": {
"manifest": {
"containers": [
{
"env": [
{
"key": "STORAGE_PATH",
"name": "STORAGE_PATH",
"value": "/tmp/openshift.local.registry"
},
{
"key": "OPENSHIFT_URL",
"name": "OPENSHIFT_URL",
"value": "http://172.17.42.1:8080/osapi/v1beta1"
},
{
"key": "REGISTRY_URL",
"name": "REGISTRY_URL",
"value": "172.121.17.1:5001"
}
],
"image": "registry",
"imagePullPolicy": "",
"name": "registry-container",
"ports": [
{
"containerPort": 5000,
"protocol": "TCP"
}
],
"volumeMounts": [
{
"mountPath": "/tmp/openshift.local.registry",
"name": "registry-storage",
"path": "/tmp/openshift.local.registry"
}
]
}
],
"id": "",
"restartPolicy": {
"always": {}
},
"version": "v1beta1",
"volumes": [
{
"name": "registry-storage",
"source": {
"emptyDir": null,
"hostDir": {
"path": "/tmp/openshift.local.registry"
},
"persistentDisk": null
}
}
]
}
},
"labels": {
"deployment": "docker-registry-3",
"deploymentConfig": "docker-registry",
"name": "registrypod"
}
},
"replicaSelector": {
"name": "registrypod"
},
"replicas": 1
},
"creationTimestamp": "2014-11-07T04:06:45Z",
"details": {
"causes": [
{
"type": "ConfigChange"
}
]
},
"id": "docker-registry-3",
"kind": "Deployment",
"labels": {
"deploymentConfig": "docker-registry"
},
"namespace": "test",
"resourceVersion": 62,
"selfLink": "/osapi/v1beta1/deployments/docker-registry-3",
"status": "Complete",
"strategy": {
"type": "Basic"
}
} |
|
Submitted a fix for the unnecessary deployments here: #366 |
csrwng
force-pushed
the
go_build_scripts_2
branch
from
1af5874
to
8da7af8
Compare
|
Rebased. [test] |
|
Evaluated for origin up to 8da7af8 |
atomic-openshift-3.3.0.24-1
generate informers with upstream informer-gen
Builder images converted to go commands. Main builder code in pkg/build/builder