Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate buildconfig for Openshift #206

Merged
merged 33 commits into from
Dec 28, 2016
Merged

Conversation

rtnpro
Copy link
Contributor

@rtnpro rtnpro commented Oct 13, 2016

This implements support for generating buildconfig for services on Openshift.

@dustymabe
Copy link
Contributor

rebase please

@rtnpro rtnpro force-pushed the buildconfig branch 4 times, most recently from b286f9f to 795535f Compare October 19, 2016 08:08
Copy link
Contributor Author

@rtnpro rtnpro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some areas for improvement.

@@ -23,7 +23,7 @@ import (
)

var unsupportedKey = map[string]int{
"Build": 0,
// "Build": 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to have provider based checks for unsupported keys, as, build is now supported for openshift but not for kubernetes.

Ref: "master",
URI: getGitRemote("origin"),
},
ContextDir: "./",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value for ContextDir cannot be value of build or build.context from Docker Compose file. There may be cases where the compose file is not at the root directory of the project. In that case, we evaluate the path relative from the root directory of the project, instead, of the relative path from the compose file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also case (and this is most cases) where build context is in dir, so we have to detect where this dir is relative to root of git repo.
Look at this example: https://github.com/kadel/compose-build-test

@rtnpro rtnpro changed the title [WIP] Generate buildconfig for Openshift Generate buildconfig for Openshift Oct 19, 2016
@@ -55,6 +57,15 @@ func getImageTag(image string) string {
}
}

// getGitRemote gets git remote URI for the current git repo
func getGitRemote(remote string) string {
out, err := exec.Command("git", "remote", "get-url", remote).Output()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about this :-(
Is it possible to do it without calling external binary?

Copy link
Member

@surajssd surajssd Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth looking how OpenShift/oc is doing it! Maybe reading from .git/config in root directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are sticking to os/exec as used by openshift itself.

if err != nil {
return ""
}
return string(out)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is returning string with \n at the end. Or at least this is what i got in BuildConfig spec.source.git.uri

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Ref: "master",
URI: getGitRemote("origin"),
},
ContextDir: "./",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also case (and this is most cases) where build context is in dir, so we have to detect where this dir is relative to root of git repo.
Look at this example: https://github.com/kadel/compose-build-test

{Type: "ImageChange"},
},
// RunPolicy
"serial",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has to be "Serial"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@@ -150,6 +207,10 @@ func (k *OpenShift) Transform(komposeObject kobject.KomposeObject, opt kobject.C
objects = append(objects, initImageStream(name, service))
}

if opt.CreateBuildConfig && service.Build != "" {
objects = append(objects, initBuildConfig(name, service)) // Openshift BuildConfigs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to modify ImageStream that is created few lines above, you cannot use it as it is right now with builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@rtnpro
Copy link
Contributor Author

rtnpro commented Oct 29, 2016

@dustymabe 23536de fixes the issue you reported!

@rtnpro
Copy link
Contributor Author

rtnpro commented Oct 29, 2016

@kadel @surajssd could you review this pull request once again? The generated artifacts now work with openshift as is. I am looking at how to replace doing system calls to git. I need git for 2 things:

  • to find out the relative path of the compose file directory root dir of the project using git rev-parse --show-prefix. I don't have a way to work around this without git or git2go
  • to find the remote url of the project repository. It can be worked around by reading .git/config

@ngtuna
Copy link
Contributor

ngtuna commented Oct 30, 2016

Would you like to bring this PR to the release ? @kadel @rtnpro @surajssd

@kadel
Copy link
Member

kadel commented Oct 31, 2016

@rtnpro Sorry, I didn't have much time latlely. But I'll will look at this as soon as I can.

@ngtuna I dont' think that we are going to make this to today's release

@surajssd
Copy link
Member

surajssd commented Nov 7, 2016

@rtnpro so I tried to build on the latest branch today and saw that user can provide custom remote using --repo https://github.com/rtnpro/kompose.git --branch buildconfig but then how do you know what service's remote to change?

A docker-compose file can have multiple services and each service can be a project in itself!

@surajssd
Copy link
Member

@rtnpro I tried this build on OpenShift online

Convert:

$ kompose --provider openshift convert --stdout -y --bc > output.yml
WARN[0000] [foo] Service cannot be created because of missing port. 

and create objects in online

$ oc new-project alm
$ oc create -f output.yml 
deploymentconfig "foo" created
imagestream "foo" created
Error from server: buildconfigs "foo" is forbidden: build strategy Docker is not allowed

It's not allowing build-strategy docker in openshift online.

Will give it one try on OpenShift running locally.

@rtnpro rtnpro force-pushed the buildconfig branch 2 times, most recently from dd0cc68 to 724930b Compare November 21, 2016 19:17
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2016
@rtnpro rtnpro force-pushed the buildconfig branch 2 times, most recently from fda463e to 1115315 Compare November 22, 2016 10:13
@kadel
Copy link
Member

kadel commented Nov 28, 2016

Can you please rebase.
It looks like there are some govet errors.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 46.582% when pulling 8ed07fc on rtnpro:buildconfig into c80735c on kubernetes-incubator:master.

@surajssd
Copy link
Member

@rtnpro two things now

$ kompose --provider openshift convert -o config/ --build-repo https://github.com/rtnpro/kompose 
WARN[0000] [foo] Service cannot be created because of missing port. 
INFO[0000] Buildconfig using https://github.com/rtnpro/kompose:: as source. 
<snip>

It is correctly reading the branch but then it is not showing up as you can see in last line.

It is still not pushing to internal registry!

I see following things in status

$ oc status -v
In project simple on server https://192.168.121.103:8443

dc/foo deploys istag/foo:latest <- bc/foo docker builds https://github.com/rtnpro/kompose#buildconfig 
    build #1 failed 2 minutes ago - 8ed07fc: Fixed typos in openshift buildconfig. (Ratnadeep Debnath <rtnpro@gmail.com>)
  deployment #1 waiting on image or update

Errors:
  * build/foo-1 has failed.
    try: Inspect the build failure with 'oc logs -f bc/foo'
Warnings:
  * pod/foo-1-build has no liveness probe to verify pods are still running.
    try: oc set probe pod/foo-1-build --liveness ...
  * The image trigger for dc/foo will have no effect until istag/foo:latest is imported or created by a build.
  * dc/foo has no readiness probe to verify pods are ready to accept traffic or ensure deployment is successful.
    try: oc set probe dc/foo --readiness ...
  * dc/foo has no liveness probe to verify pods are still running.
    try: oc set probe dc/foo --liveness ...

View details with 'oc describe <resource>/<name>' or list everything with 'oc get all'.

and build container failed saying

$ oc logs foo-1-build
Cloning "https://github.com/rtnpro/kompose" ...
        Commit: 8ed07fca52fee936c6ef6d3117aac661bfd085ab (Fixed typos in openshift buildconfig.)
        Author: Ratnadeep Debnath <rtnpro@gmail.com>
        Date:   Wed Dec 28 16:58:52 2016 +0530
Step 1 : FROM busybox
 ---> 1efc1d465fd6
<snip>
Removing intermediate container 1fb8d9987023
Successfully built 4e0fdc19432e
Pushing image 172.30.240.241:5000/simple/foo:latest ...
error: build error: Failed to push image: unauthorized: authentication required

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 46.582% when pulling b10c0ad on rtnpro:buildconfig into c80735c on kubernetes-incubator:master.

- spelling mistake
- pass compose file dir instead of compose file to initBuildConfig call
- Use as default value for cli --build-branch option
- Pass current build branch to buildconfig related functions instead of opt.BuildBranch
- Fix printing buildconfig source branch in logs.
- Added buildconfig doc in user guide.
- Add inline code documentation to explain why buildconfig
  object needs to be created after imagestream, because of
  openshift/origin#4518
@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 46.582% when pulling 0d86f3e on rtnpro:buildconfig into c80735c on kubernetes-incubator:master.

@surajssd surajssd added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 28, 2016
@surajssd surajssd dismissed kadel’s stale review December 28, 2016 15:37

All your concerns were addressed!

@surajssd
Copy link
Member

@rtnpro @kadel great work and thanks for all the patience 👍

@surajssd surajssd merged commit 9c3fdaa into kubernetes:master Dec 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. need 2nd review review needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants