Add option to add/remove a specific port/env var of a specific container#131
Add option to add/remove a specific port/env var of a specific container#131yangcao77 merged 6 commits intodevfile:mainfrom
Conversation
yangcao77
left a comment
yangcao77
left a comment
There was a problem hiding this comment.
Changes look good. Just need to update the function description for the new behavior.
pkg/devfile/parser/configurables.go
Outdated
| @@ -80,14 +81,18 @@ func (d DevfileObj) SetPorts(ports ...string) error { | |||
| } | |||
|
|
|||
| // RemovePorts removes all container endpoints from a devfile | |||
There was a problem hiding this comment.
this function description should be updated
pkg/devfile/parser/configurables.go
Outdated
| @@ -61,16 +61,17 @@ func (d DevfileObj) RemoveEnvVars(keys []string) (err error) { | |||
| } | |||
|
|
|||
| // SetPorts converts ports to endpoints, adds to a devfile | |||
There was a problem hiding this comment.
Please update the description for the new behavior
…ainer Signed-off-by: Parthvi Vala <[email protected]>
Signed-off-by: Parthvi Vala <[email protected]>
valaparthvi
force-pushed
the
fix-odo-5423
branch
from
44b28aa to
f3d5054
Compare
pkg/devfile/parser/configurables.go
Outdated
| // now check if the port(s) requested for removal exists in | ||
| // the ports set for the component | ||
| for _, port := range ports { | ||
| if !InArray(portList, port) { |
There was a problem hiding this comment.
Can replace portList to a map. the key look up for a map is O(1), and the InArray here is looking for a key in Array, which will need to iterate through the array again and is slower.
Also, since you are iterating the ports array here, you can also store the ports information in a map, and replace the below InArray lookup when doing the endpoints removal.
There was a problem hiding this comment.
Can you also help update the initial InArray usage in RemoveEnvVarsFromList? and also the InArray function? Thanks.
pkg/devfile/parser/configurables.go
Outdated
| // SetPorts converts ports to endpoints, adds to a devfile | ||
| func (d DevfileObj) SetPorts(ports ...string) error { | ||
| // SetPorts accepts a map of container name and the port numbers to be set; | ||
| // it converts ports to endpoints, and adds to a devfile |
There was a problem hiding this comment.
please update the description as this PR changes the existing behavior. currently it will set the ports to all container components, and this change will look for specific component and add the endpoint.
Signed-off-by: Parthvi Vala <[email protected]>
…omponent Signed-off-by: Parthvi Vala <[email protected]>
|
/hold |
|
/unhold |
|
@yangcao77 I have modified the existing devfileObj methods and added a few new methods to the |
valaparthvi
changed the title
valaparthvi
force-pushed
the
fix-odo-5423
branch
from
c811d97 to
d3abc9d
Compare
Signed-off-by: Parthvi Vala <[email protected]>
valaparthvi
force-pushed
the
fix-odo-5423
branch
from
d3abc9d to
cbc2971
Compare
| corev1 "k8s.io/api/core/v1" | ||
| ) | ||
|
|
||
| func (d *DevfileV2) AddEnvVars(containerEnvMap map[string][]v1alpha2.EnvVar) error { |
There was a problem hiding this comment.
Can you add some description comments for those exposed functions?
Signed-off-by: Parthvi Vala <[email protected]>
yangcao77
left a comment
yangcao77
left a comment
There was a problem hiding this comment.
Changes look good to me.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: valaparthvi, yangcao77 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
valaparthvi
changed the title
2 participants
What does this PR do?:
This PR modifies devfileObj methods such as SetPorts to allow setting port to a specific container, and RemovePorts to remove a specific port of a specific container.
Which issue(s) this PR fixes:
redhat-developer/odo#5423
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:
The unit tests should help give an idea about the PR changes.