#682 APP_PORT not injected into app container #685

akhilac1 · 2022-06-04T07:44:15Z

Signed-off-by: akhilac1 chetlapalle.akhila@gmail.com

Description

Added default value for APP_PORT in case it cannot be fetched from environment variables
Updated yaml to include addition of environment variable for APP_PORT [ until the dapr.io/app_port annotation issue is fixed dapr.io/app_port annotation does not get injected into environment variable in K8s mode #684 ]
Default command to run the app in observability quickstart ALWAYS uses port 5000. Updated quickstarts/tutorials/observability/python/Dockerfile to use python3 app.py as launch command and use specified port

Issue reference

We strive to have all PRs being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #682

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

The quickstart code compiles correctly
You've tested new builds of the quickstart if you changed quickstart code
You've updated the quickstart's README if necessary
If you have changed the steps for a quickstart be sure that you have updated the automated validation accordingly. All of our quickstarts have annotations that allow them to be executed automatically as code. For more information see mechanical-markdown. For user guide with examples see Examples.

Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

…lac1/quickstarts into hotfix_quickstarts_app_port

Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

…lac1/quickstarts into hotfix_quickstarts_app_port Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

akhilac1 · 2022-06-04T13:34:22Z

@paulyuk @msfussell @berndverst - The ubuntu tutorials are failing due to python app in distributed calculator. This app needs the updated docker image to pass - since the existing docker image uses flask and the app never comes up on 5001. It works on my local test with updated Docker image. My understanding is that test does not build Docker files - rather it is using old Docker file in which case this test would not pass until the image is updated.
Also am unable to fix the DCO error due to merge conflicts [ added and reverted some code]. Can this be overridden by the maintainer and merged?

Attaching my local execution log for reference -
distributedCaculator_multiplyPod.txt

paulyuk · 2022-06-05T08:14:31Z

Hi @akhilac1 - thank you for this. The DCO issue is separate from the Docker build and push to registry. For DCO I suggest you do every commit via a git commit -s -p or at least -s as a part of your commit. This will sign with DCO and pass that. I will sign it this time. Also you're right there's a chicken and egg problem here. The action as written needs the docker images for python, distribute calculator, etc published to GHCR before the tests will pass. So basically we test these in prod, and rely on the review to decide if we should let it in. I will do that now.

paulyuk

Hi - just a few things to look at please. thank you for updating all this.

paulyuk · 2022-06-05T08:28:54Z

pub_sub/javascript/http/order-processor/index.js

-    console.log('[error]: --app-port is not set. Re-run dapr run with -p or --app-port.\nUsage: https://docs.dapr.io/getting-started/quickstarts/pubsub-quickstart/\n');
-    process.exit(1);
-}
+const APP_PORT = process.env.APP_PORT || 5001;


Suggested change

const APP_PORT = process.env.APP_PORT || 5001;

const APP_PORT = process.env.APP_PORT ?? '5001';

Prefer we use ?? operator which covers more cases for false that come up in Javascript. I. believe we want port to be a string too.

tutorials/bindings/nodeapp/app.js

tutorials/distributed-calculator/node/app.js

paulyuk · 2022-06-05T08:38:53Z

tutorials/distributed-calculator/python/app.py

@@ -32,4 +30,4 @@ def multiply():
    print(f"Calculating {operand_one} * {operand_two}", flush=True)
    return jsonify(math.ceil(operand_one * operand_two * 100000)/100000)

-app.run(port=appPort)
+app.run(host="0.0.0.0",port=appPort)


Why do we need "0.0.0.0" host ip?

@paulyuk - to make the value being used explicit.

tutorials/hello-world/node/app.js

paulyuk · 2022-06-05T08:40:24Z

tutorials/observability/python/app.py

@@ -37,4 +35,4 @@ def multiply():
    print(f"Calculating {operand_one} * {operand_two}", flush=True)
    return jsonify(math.ceil(operand_one * operand_two * 100000)/100000)

-app.run(port)
+app.run(host="0.0.0.0",port=port)


Just checking we need to set host?

@paulyuk - It is not mandatory. This makes the value being used explicit.

@akhilac1 ok, I just want to make sure that still works in a few environments that might have benefited from defaults. E.g. have you tried this on Windows, WSL, and Mac? have you tried with Ipv6?

The other way to ask is are we fixing a problem with this change? if not we might have lower risk of regressions by not making the change, since this has worked this way for a few years.

That said, I'll take it and let's monitor

Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>

paulyuk

LGTM

paulyuk · 2022-06-05T20:21:06Z

Tests are all clean except for Validate Tutorials which is expected. I'm merging.

akhilac1 and others added 8 commits June 4, 2022 12:59

dapr#682 APP_PORT not injected into app container

78caa02

Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

Update app.py

95bc628

dapr#682 APP_PORT not injected into app container

2917cbf

Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

Merge branch 'hotfix_quickstarts_app_port' of https://github.com/akhi…

11c10d9

…lac1/quickstarts into hotfix_quickstarts_app_port

Update app.py

4ea5640

Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

Merge branch 'hotfix_quickstarts_app_port' of https://github.com/akhi…

b866cd6

…lac1/quickstarts into hotfix_quickstarts_app_port Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

clean up test commands from yaml

8d50f5d

Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

revert python launch command

487ddaa

Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

paulyuk requested changes Jun 5, 2022

View reviewed changes

akhilac1 and others added 4 commits June 5, 2022 18:43

Update pub_sub/javascript/http/order-processor/index.js

ec4d0a7

Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>

Update tutorials/distributed-calculator/node/app.js

cd282dc

Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>

Update tutorials/hello-world/node/app.js

7787b0c

Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>

Update tutorials/bindings/nodeapp/app.js

69efc77

Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>

paulyuk approved these changes Jun 5, 2022

View reviewed changes

paulyuk merged commit 2f37181 into dapr:master Jun 5, 2022

yaron2 assigned akhilac1 Jul 6, 2022

yaron2 added this to the 1.8 milestone Jul 6, 2022

akhilac1 mentioned this pull request Oct 12, 2022

REQUEST: New membership for akhilac1 dapr/community#233

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#682 APP_PORT not injected into app container #685

#682 APP_PORT not injected into app container #685

akhilac1 commented Jun 4, 2022

akhilac1 commented Jun 4, 2022 •

edited

paulyuk commented Jun 5, 2022

paulyuk left a comment

paulyuk Jun 5, 2022

paulyuk Jun 5, 2022

akhilac1 Jun 5, 2022

paulyuk Jun 5, 2022

akhilac1 Jun 5, 2022

paulyuk Jun 5, 2022

paulyuk Jun 5, 2022

paulyuk left a comment

paulyuk commented Jun 5, 2022

	const APP_PORT = process.env.APP_PORT \|\| 5001;
	const APP_PORT = process.env.APP_PORT ?? '5001';

#682 APP_PORT not injected into app container #685

#682 APP_PORT not injected into app container #685

Conversation

akhilac1 commented Jun 4, 2022

Description

Issue reference

Checklist

akhilac1 commented Jun 4, 2022 • edited

paulyuk commented Jun 5, 2022

paulyuk left a comment

Choose a reason for hiding this comment

paulyuk Jun 5, 2022

Choose a reason for hiding this comment

paulyuk Jun 5, 2022

Choose a reason for hiding this comment

akhilac1 Jun 5, 2022

Choose a reason for hiding this comment

paulyuk Jun 5, 2022

Choose a reason for hiding this comment

akhilac1 Jun 5, 2022

Choose a reason for hiding this comment

paulyuk Jun 5, 2022

Choose a reason for hiding this comment

paulyuk Jun 5, 2022

Choose a reason for hiding this comment

paulyuk left a comment

Choose a reason for hiding this comment

paulyuk commented Jun 5, 2022

akhilac1 commented Jun 4, 2022 •

edited