Allow nodePort value for the postgres-operator-ui service

[marcusportmann]

Added support for specifying a nodePort value for the postgres-operator-ui service when the service type is NodePort.

[FxKu]

I think I would rather add the nodePort field to the values file and check if the service type is set to "NodePort" instead of checking if nodePort field is set.

[marcusportmann]

I have made the change you suggested. I think it is still important to see if a node port value is specified because a randomised value may be desired in some cases. If both the type is set to NodePort and the value is specified then it will be applied.

[FxKu]

Could you still add the nodePart field in the values.yaml (and values-crd.yaml) without a value? Would it then receive a random port?

[marcusportmann]

I have added a nodePort field to the values.yaml file with no value. This is for the postgres-operator-ui chart so there is no values-crd.yaml. If no value is specified for this field a random port is selected. If a value is specified for this field it is used as the NodePort port.

[FxKu]

Will the nodePort field be ignored when the service is of type ClusterIP?

[marcusportmann]

Adding the nodePort field to the values.yaml file causes the Travis CI build to fail. I attempted to remove the conditional addition of the field in the Helm chart to remedy this but the build unfortunately still fails. Both scenarios work for me when I test the operator locally when the type of the service is NodePort. Further testing when the type of the service is ClusterIP causes the build to fail if the nodePort field is specified. As a result, I am reverting my changes since it feels wrong to add a field that is conditional to the values.yaml file when it is not applicable i.e. the type for the service is not NodeType. I will add the check back to the Helm chart and a commented out field to the values.yaml file to indicate the field does exist and can be specified when the type is NodePort.

[FxKu]

The build error has nothing to do with this PR. Besides, the UI isn't really covered in our tests.
It's fine for me to have the blank nodePort field in the values.yaml. If it's ignored for ClusterIP service than we can save the conditional test in the service template. I like that.

Edit: Ok now I've read your edited message again. If it fails when the nodePort is specified then it's true that we have to check that the serviceType is not of ClusterIP. I would still keep that field in the values.yaml then.

[marcusportmann]

Are you happy with having the nodePort field commented out in the values.yaml file with a comment describing its usage?

[FxKu]

Why not add if type != clusterIP to the service template, similar to what you had before?

[marcusportmann]

Because the field is only valid if the type is NodePort. Removing the check in the service.yaml demonstrated this as the deployment failed. I believe there is a difference between an optional field, for which an empty value is valid, and a conditional field that is only valid as a result of the value of some other field. Having it commented out with a comment explaining its usage makes it clear that just setting the value is not sufficient and that the type also needs to be set to NodePort.

[FxKu]

👍

[sdudoladov]

👍

[sdudoladov]

@marcusportmann thank you for the contribution :)