-
Notifications
You must be signed in to change notification settings - Fork 752
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
Support multiple hostnames on expose label in service for Kubernetes #1092
Conversation
…d by semicolon (;)
I believe we should use comma (,) rather than semicolon (;). I rarely see semicolons be used. Could you also test against using, for example, spaces in between? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few concerns, otherwise the code + tests look great!
@@ -324,6 +324,8 @@ func (k *Kubernetes) InitDS(name string, service kobject.ServiceConfig) *extensi | |||
|
|||
func (k *Kubernetes) initIngress(name string, service kobject.ServiceConfig, port int32) *extensions.Ingress { | |||
|
|||
hosts := strings.Split(service.ExposeService, ";") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also use some form of checking (making sure spaces are removed, that someone doesn't add: ,,
for example..
Oh! Could you add some documentation too for when this is merged? We should indicate to the users that multiple values are allowed. |
… / trailing spaces and repeated commas; Updated test cases and documents.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Updated. Please take a look at it. |
Awesome on the tests! 👍 and the code LGTM. Thanks for the contribution! |
/lgtm |
Support multiple hostnames [separated by semicolon] on expose label (kompose.service.expose) in service for Kubernetes.