koki / short

Manageable Kubernetes manifests through a composable, reusable syntax
https://docs.koki.io/short
Apache License 2.0
122 stars 14 forks source link

Service port when only set in the ports field doesn't work #54

Closed wlan0 closed 6 years ago

wlan0 commented 6 years ago
 service:
   version: v1
   name: docs
   #port: 80:3000
   selector:
      app: docs.koki.io
   type: LoadBalancer
   ports:
   - 80:3000 

The above yaml doesn't parse into a kubernetes type, but this does

service:
  version: v1
  name: docs
  port: 80:3000
  selector:
    app: docs.koki.io
  type: LoadBalancer
  #ports:
  # - 80:3000

Both should work

ublubu commented 6 years ago

I think this might be a documentation issue. ports is a dictionary of named ports. If you just want one unnamed port, then use the port field.

wlan0 commented 6 years ago

It's not a documentation issue. If the user chooses to follow this style, they should be allowed to do so.

No documentation can be effective at communicating such nuances. It's best to support both styles.

ublubu commented 6 years ago

My issue with allowing the list format is that they won't be able to add more ports to the list.

In general, I don't think we should have multiple syntaxes for the same thing, especially if one syntax can trick them into thinking an invalid use case is valid.

wlan0 commented 6 years ago

Then how do you explain port: and node_port:

This is another syntax to specify ports (the same thing). This is what caused the original problem. Remove those two fields.

ublubu commented 6 years ago

I'm ok with that solution (removing port and node_port). I'll add a good error message for anyone trying to add extra unnamed ports to the list

wlan0 commented 6 years ago

Thanks

ublubu commented 6 years ago

I just realized there isn't an obvious place to put NodePort for an anonymous ServicePort:

# before
port: 80:3000
node_port: 12345

# after
ports:
- 80:3000 # This is a string, so we can't add a 'node_port' field.

Named ports don't have this issue because they're dictionaries, not strings:

ports:
- port_name_foo: 80:3000
  node_port: 12345

If we put the anonymous port in the ports list, it'll need to have a different format than the named ports. I'm not sure that having separate formats is better than having separate fields (port vs ports).

I think it's best to leave the functionality as-is for now.

wlan0 commented 6 years ago

SGTM