change debugport in container if its different from env_info #4785

girishramnani · 2021-06-08T16:33:28Z

What type of PR is this?
/kind bug

What does this PR do / why we need it:
change debugport in container if its different from env_info, also added debug command for converted devfiles.

Which issue(s) this PR fixes:

Fixes #4615

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

mik-dass · 2021-06-09T07:45:40Z

pkg/devfile/adapters/kubernetes/utils/utils.go

+				for i, envVar := range container.Env {
+					if envVar.Name == adaptersCommon.EnvDebugPort {
+						if envVar.ValueFrom == nil && (envVar.Value != strconv.Itoa(devfileDebugPort)) {
+							container.Env[i].Value = strconv.Itoa(devfileDebugPort)
+							break


Please add a test case here https://github.com/openshift/odo/blob/ca5ebd60331eaa32bc5667ca59ac2e010707c369/pkg/devfile/adapters/kubernetes/utils/utils_test.go#L161

mik-dass

/lgtm

girishramnani · 2021-06-10T10:48:33Z

@mik-dass the lgtmer has to approve also, please can approve it as well.

mik-dass · 2021-06-10T10:53:30Z

@mik-dass the lgtmer has to approve also, please can approve it as well.

I think, in a recent discussion, it was decided to have two reviewers on each PR.

girishramnani · 2021-06-10T11:08:06Z

@mik-dass the lgtmer has to approve also, please can approve it as well.

I think, in a recent discussion, it was decided to have two reviewers on each PR.

Ack, thank you for reminding

dharmit · 2021-06-11T06:39:38Z

@girishramnani I followed the below steps:

$ git clone https://github.com/odo-devfiles/nodejs-ex
$ cd nodejs-ex
$ odo create --s2i nodejs node-s2i

$ odo env set DebugPort 8888
$ cat .odo/env/env.yaml
ComponentSettings:
  Name: node-s2i
  Project: myproject
  AppName: app
  DebugPort: 8888
  RunMode: run

$ odo push

$ odo exec -- env | grep DEBUG
DEBUG_PORT=5858

$ grep DEBUG_PORT devfile.yaml
    - name: DEBUG_PORT
      value: "5858"

There are two concerns I have:

Pushed component's env var still shows 5858 as the DebugPort
devfile contains 5858 as the value for DEBUG_PORT

DebugPort and DEBUG_PORT are the same things, right? Or am I looking at this the wrong way?

Am I testing this the right way?

cc @kadel

dharmit · 2021-06-11T06:43:52Z

DebugPort and DEBUG_PORT are the same things, right? Or am I looking at this the wrong way?

#4615 (comment) gives me an impression that these two are different things. 😕

girishramnani · 2021-06-11T11:51:02Z

@girishramnani I followed the below steps:
$ git clone https://github.com/odo-devfiles/nodejs-ex
$ cd nodejs-ex
$ odo create --s2i nodejs node-s2i

$ odo env set DebugPort 8888
$ cat .odo/env/env.yaml
ComponentSettings:
  Name: node-s2i
  Project: myproject
  AppName: app
  DebugPort: 8888
  RunMode: run

$ odo push

$ odo exec -- env | grep DEBUG
DEBUG_PORT=5858

$ grep DEBUG_PORT devfile.yaml
    - name: DEBUG_PORT
      value: "5858"
There are two concerns I have:

Pushed component's env var still shows 5858 as the DebugPort

devfile contains 5858 as the value for DEBUG_PORT

DebugPort and DEBUG_PORT are the same things, right? Or am I looking at this the wrong way?

Am I testing this the right way?

cc @kadel

@dharmit the debug port only gets changed in devfile when you provide odo push --debug.

You can still see the debug port in the env because its part of the original env of the s2i and gets passed to the devfile.

Please follow the same steps but with odo push --debug and you will see that the debug port gets changed in the pod.

dharmit · 2021-06-11T12:25:15Z

@dharmit the debug port only gets changed in devfile when you provide odo push --debug.
You can still see the debug port in the env because its part of the original env of the s2i and gets passed to the devfile.
Please follow the same steps but with odo push --debug and you will see that the debug port gets changed in the pod.

That works. But should we also change the DEBUG_PORT value in devfile.yaml when user does odo env set DebugPort? I am asking this because there are different values (and even different naming style - DEBUG_PORT vs. DebugPort) for debug port in devfile vs. that in env.yaml.

I think env.yaml is for environment specific information and it can have values different from devfile. But I would like to be sure.

dharmit · 2021-06-11T12:27:27Z

@dharmit the debug port only gets changed in devfile when you provide odo push --debug.

From UX perspective, how do we convey this to the user that they must do odo push --debug for this env var value to actually take effect in the component? Or should the user be aware of it already since odo push --debug is the only way to start the component in debug mode?

girishramnani · 2021-06-11T17:03:02Z

@dharmit the debug port only gets changed in devfile when you provide odo push --debug.

From UX perspective, how do we convey this to the user that they must do odo push --debug for this env var value to actually take effect in the component? Or should the user be aware of it already since odo push --debug is the only way to start the component in debug mode?

This been a standard set in devfile already since a while now

girishramnani · 2021-06-11T17:09:12Z

@dharmit the debug port only gets changed in devfile when you provide odo push --debug.
You can still see the debug port in the env because its part of the original env of the s2i and gets passed to the devfile.
Please follow the same steps but with odo push --debug and you will see that the debug port gets changed in the pod.

That works. But should we also change the DEBUG_PORT value in devfile.yaml when user does odo env set DebugPort? I am asking this because there are different values (and even different naming style - DEBUG_PORT vs. DebugPort) for debug port in devfile vs. that in env.yaml.

I think env.yaml is for environment specific information and it can have values different from devfile. But I would like to be sure.

Technically you can but changing debug port from what it is using odo env set DebugPort probably feels like it should affect env.yml and doesn’t get persisted to devfile.

its feels more of a tool to override the value in devfile for your local setup only?

dharmit · 2021-06-14T07:16:14Z

@dharmit the debug port only gets changed in devfile when you provide odo push --debug.

Not sure if I'm nitpicking here. Let me know if so.

When I do odo push --debug, the debugport env does reflect correctly in the pod. But devfile still shows 5858. Based on above comment, I expected it to change in devfile as well. Or is that a wrong expectation?

dharmit · 2021-06-14T07:19:26Z

@dharmit the debug port only gets changed in devfile when you provide odo push --debug.

From UX perspective, how do we convey this to the user that they must do odo push --debug for this env var value to actually take effect in the component? Or should the user be aware of it already since odo push --debug is the only way to start the component in debug mode?

This been a standard set in devfile already since a while now

OK. I find it unintuitive that user isn't notified about using odo push --debug. I'm not sure if this is something we should cover in the docs or in the cli messaging.

Just to be clear, I don't mean we address it in this PR. We should do it separately.

dharmit · 2021-06-14T07:20:26Z

Technically you can but changing debug port from what it is using odo env set DebugPort probably feels like it should affect env.yml and doesn’t get persisted to devfile.

Agree. odo env commands shouldn't touch anything in devfile. That would be an even bigger problem. 👍🏾

dharmit · 2021-06-14T07:21:11Z

/approve
/lgtm

Doing both since lgtm was removed by rebase - #4785 (comment)

/override ci/prow/psi-unit-test-mac

openshift-ci · 2021-06-14T07:21:14Z

@dharmit: Overrode contexts on behalf of dharmit: ci/prow/psi-unit-test-mac

In response to this:

/approve
/lgtm

Doing both since lgtm was removed by rebase - #4785 (comment)

/override ci/prow/psi-unit-test-mac

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-06-14T07:21:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharmit

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:

~~OWNERS~~ [dharmit]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

change debugport in container if its different from env_info

6965c1e

openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 8, 2021

update changelog

d830ac9

openshift-ci bot requested review from dharmit and rnapoles-rh June 8, 2021 16:34

mik-dass reviewed Jun 9, 2021

View reviewed changes

add unit test case

5e6235b

mik-dass approved these changes Jun 10, 2021

View reviewed changes

openshift-ci bot assigned mik-dass Jun 10, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 10, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jun 13, 2021

Merge branch 'main' into debug_port_fix

16d98c0

openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. Required by Prow. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Jun 13, 2021

openshift-ci bot assigned dharmit Jun 14, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 14, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 14, 2021

openshift-merge-robot merged commit a08eec3 into redhat-developer:main Jun 14, 2021

girishramnani deleted the debug_port_fix branch June 14, 2021 08:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change debugport in container if its different from env_info #4785

change debugport in container if its different from env_info #4785

girishramnani commented Jun 8, 2021 •

edited

mik-dass Jun 9, 2021

girishramnani Jun 9, 2021

mik-dass left a comment

girishramnani commented Jun 10, 2021

mik-dass commented Jun 10, 2021

girishramnani commented Jun 10, 2021

dharmit commented Jun 11, 2021 •

edited

dharmit commented Jun 11, 2021

girishramnani commented Jun 11, 2021

dharmit commented Jun 11, 2021

dharmit commented Jun 11, 2021

girishramnani commented Jun 11, 2021

girishramnani commented Jun 11, 2021

dharmit commented Jun 14, 2021

dharmit commented Jun 14, 2021

dharmit commented Jun 14, 2021

dharmit commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

change debugport in container if its different from env_info #4785

change debugport in container if its different from env_info #4785

Conversation

girishramnani commented Jun 8, 2021 • edited

mik-dass Jun 9, 2021

Choose a reason for hiding this comment

girishramnani Jun 9, 2021

Choose a reason for hiding this comment

mik-dass left a comment

Choose a reason for hiding this comment

girishramnani commented Jun 10, 2021

mik-dass commented Jun 10, 2021

girishramnani commented Jun 10, 2021

dharmit commented Jun 11, 2021 • edited

dharmit commented Jun 11, 2021

girishramnani commented Jun 11, 2021

dharmit commented Jun 11, 2021

dharmit commented Jun 11, 2021

girishramnani commented Jun 11, 2021

girishramnani commented Jun 11, 2021

dharmit commented Jun 14, 2021

dharmit commented Jun 14, 2021

dharmit commented Jun 14, 2021

dharmit commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

girishramnani commented Jun 8, 2021 •

edited

dharmit commented Jun 11, 2021 •

edited