Configure service types #189
Conversation
| delete(service.Annotations, "kompose.service.cloud") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
should we do any error checking for invalid values here? i.e. anything that is not True/true/ ? Is "False" or "false" acceptable?
There was a problem hiding this comment.
@dustymabe it now accepts "False" or "false" .
| @@ -263,6 +274,7 @@ func UpdateKubernetesObjects(name string, service kobject.ServiceConfig, objects | |||
| // Configure annotations | |||
| annotations := transformer.ConfigAnnotations(service) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
random added line. I would delete
There was a problem hiding this comment.
Deleted random line.
712ca34
to
96e9996
Compare
|
Thank you @procrypt for staring on this. I have few notes regarding this: Another question is if labels should use k8s terminology like (LoadBalancer,NodePort,ClusterIP) or something different that is more general. Any thought on this @sebgoa ? Examples:
generic:
|
|
I'd rather stick to k8s terminology than create a kompose specific one. if some providers don't use that terminology, the onus should be on them to make the mapping ;) |
|
@kadel I agree with @sebgoa we should use the k8 terminology. |
1be9fb1
to
50a517f
Compare
|
@procrypt you will have to update the unit tests and also add new functional tests for this kinda behavior. |
There was a problem hiding this comment.
@procrypt I added few suggestions. Feel free to ping me on irc or slack for any help or clarification.
Can you please include some tests for this?
I would also consider creating kompose.service.type as constant, so we can easily change it in the future.
| for key, value := range service.Annotations { | ||
| if key == "kompose.service.type" && (value == "NodePort" || value == "nodeport") { | ||
| svc.Spec.Type = "NodePort" | ||
| delete(service.Annotations, "kompose.service.type") |
There was a problem hiding this comment.
I would keep original Annotation. It might be useful to have this information in future for keeping track where this value came from.
| @@ -242,6 +242,22 @@ func CreateService(name string, service kobject.ServiceConfig, objects []runtime | |||
| servicePorts := ConfigServicePorts(name, service) | |||
| svc.Spec.Ports = servicePorts | |||
|
|
|||
| // Configure service types | |||
| for key, value := range service.Annotations { | |||
| if key == "kompose.service.type" && (value == "NodePort" || value == "nodeport") { | |||
There was a problem hiding this comment.
it would be better to have value test case insensitive.
you can convert it to lowercase strings.ToLower(value) and use it in if statement
| svc.Spec.Type = "LoadBalancer" | ||
| delete(service.Annotations, "kompose.service.type") | ||
| } else { | ||
| logrus.Fatalf("Unknown Service Type %s", value) |
There was a problem hiding this comment.
If there is any other label than kompose.service.type it fails on this.
It would be better to break this into two nested ifs or switch in if.
In first level you check if there is kompose.service.type key, and inside that you do else if or switch on values.
|
@kadel I have updated the PR as per your suggestion please take a look at it. |
03c82d6
to
47d3db9
Compare
|
Code looks good 👍 One last thing. Can you please add tests and docs? |
|
@kadel I'll add the tests and docs :) |
|
@kadel Thanks :) |
Fix #154
cc @surajssd @kadel