-
Notifications
You must be signed in to change notification settings - Fork 35
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
287-format pod container port name #51
287-format pod container port name #51
Conversation
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
f40a0a0
to
04e1965
Compare
…dContainerPortName
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
@mmulholl Some of the generated random strings in api tests are not valid. I have to modify them to match the schema. Please help review the test code change, thanks! |
@yangcao77 Hi, the changes you made to the parser tests look good. |
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.
suggestion for changing package alias to be less confusing between all these v1 and v2
@@ -4,8 +4,8 @@ import ( | |||
"reflect" | |||
"testing" | |||
|
|||
v1 "github.com/devfile/api/pkg/apis/workspaces/v1alpha2" | |||
"github.com/devfile/api/pkg/attributes" | |||
v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" |
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.
thinking if we should call these aliases devfileApiv2
🤔 it will be less confusing as well
pkg/devfile/generator/utils.go
Outdated
if _, exist := portMap[name]; exist { | ||
continue | ||
} |
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.
if _, exist := portMap[name]; exist { | |
continue | |
} | |
if _, exist := portMap[name]; !exist { | |
portMap[name] = true | |
containerPorts = append(containerPorts, corev1.ContainerPort{ | |
Name: name, | |
ContainerPort: portNumber, | |
Protocol: portProtocol, | |
}) | |
} |
would probably do this way but upto you
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.
updated
@@ -127,12 +128,14 @@ func TestConvertPorts(t *testing.T) { | |||
}, | |||
want: []corev1.ContainerPort{ | |||
{ | |||
Name: endpointsNames[0], | |||
Name: "8080-tcp", |
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.
Just noticed, can you update teh test case name, it seems wrong
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.
updated
Signed-off-by: Stephanie <[email protected]>
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maysunfaisal, 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 |
What does this PR do?
This PR formats the container port name to be
<portNumber>-<protocol>
, updated to use latest api repo and latest schema.What issues does this PR fix or reference?
devfile/api#287
Is your PR tested? Consider putting some instruction how to test your changes
unit tested