use endpoint name as container port name and service port name #128
Conversation
…15chars Signed-off-by: Stephanie <yangcao@redhat.com>
|
@valaparthvi can you also help verify this PR fixes the Odo issue? |
|
@yangcao77 It works like magic! Thank you for fixing it.
The svc port names, endpoint port names, and container port names are updated as expected. @kadel Since you raised the original issue, give it a go if you please. |
| @@ -347,7 +347,7 @@ const JsonSchema220 = `{ | |||
| }, | |||
| "name": { | |||
| "type": "string", | |||
| "maxLength": 63, | |||
| "maxLength": 15, | |||
There was a problem hiding this comment.
do we have corresponding updates in our API docs to show the new max length?
There was a problem hiding this comment.
the spec is the api doc
| @@ -663,7 +679,7 @@ func TestGetServiceSpec(t *testing.T) { | |||
| wantPorts []corev1.ServicePort | |||
| }{ | |||
| { | |||
| name: "multiple endpoints share the same port", | |||
| name: "multiple endpoints have different ports", | |||
There was a problem hiding this comment.
This test should cover most of our scenarios since we have validation on unique ports but should we handle the exception case as well where dedicatedPod=true? https://github.com/devfile/api/blob/main/pkg/validation/validation-rule.md#endpoints
There was a problem hiding this comment.
we do not specifially do tests for deficatedPod in library, since it is currently not supported in library. see devfile/api#670
| @@ -46,7 +46,12 @@ func convertPorts(endpoints []v1.Endpoint) []corev1.ContainerPort { | |||
| } else { | |||
| portProtocol = corev1.ProtocolTCP | |||
| } | |||
| name := fmt.Sprintf("%d-%s", portNumber, strings.ToLower(string(portProtocol))) | |||
| name := endpoint.Name | |||
There was a problem hiding this comment.
We are changing a user provided name underneath the covers if length exceeds 15 characters. It'll be good to warn them of these changes ahead of time (via cmd line, log, and/or doc)
There was a problem hiding this comment.
yes. This change is because we want to unify the service port name & the container port name. since service port does not accept starting with a number (has to be alphabet), thus we use the convention port-<number>
@valaparthvi FYI.
There was a problem hiding this comment.
Does the warning need to happen on the odo side?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kim-tsao, yangcao77 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 |
Signed-off-by: Stephanie yangcao@redhat.com
What does this PR do?:
use endpoint name as container port name and service port name if <= 15 chars. if > 15 chars (devfile spec before the change to shorten endpoint name limit), use
port-<portNumber>Which issue(s) this PR fixes:
Fixes devfile/api#700
PR acceptance criteria:
Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.
Unit/Functional tests
QE Integration test
Documentation
Client Impact
How to test changes / Special notes to the reviewer: