move git and related functions from openshift.go into a separate file #651
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
#655 will unblock this PR |
|
@surajnarwade Yes it will unblock this PR. |
|
Actually... there's a lot of things weird going on in this PR, weird indenting, changing of code to buildconfig. This all should just be a quick move and function name change, shouldn't it? |
|
|
||
| // sort all object so all services are first | ||
| o.SortServicesFirst(&allobjects) | ||
| } |
| // sort all object so all services are first | ||
| o.SortServicesFirst(&allobjects) | ||
| // If docker-compose has a volumes_from directive it will be handled here | ||
| o.VolumesFrom(&allobjects, komposeObject) |
| } | ||
| } else { | ||
| svc := o.CreateHeadlessService(name, service, objects) | ||
| objects = append(objects, svc) |
There was a problem hiding this comment.
again, changes to unrelated code
| // If ports not provided in configuration we will not make service | ||
| if o.PortsExist(name, service) { | ||
| svc := o.CreateService(name, service, objects) | ||
| objects = append(objects, svc) |
| if err != nil { | ||
| return nil, errors.Wrap(err, "Buildconfig cannot be created because current git branch couldn't be detected.") | ||
| } | ||
| } |
There was a problem hiding this comment.
weird indenting again, you indented all of this code.
|
@procrypt run gofmt |
|
@surajnarwade makes sense to put in different folder even though only OpenShift uses it.. we may use it for Kubernetes in the future? |
|
@surajnarwade we will be using it for Functional Tests also. |
|
LGTM |
|
Ping @kadel Please review. |
|
Should those functions be used in openshift code for buildconfig stuff etc...? |
|
@kadel , yeah buildconfig stuff |
|
@procrypt Mind fixing the conflicting file and then we can do another review / possible merge? |
There was a problem hiding this comment.
@surajnarwade the PR says that code is being moved, but I don't see code being removed from somewhere else, it's just code addition? can you please elaborate what is happening?
surajnarwade
changed the title
|
This LGTM. |
Fixes #640 and we can use the same functions in #518
cc: @kadel @surajssd