Skip to content

Conversation

@anhldbk
Copy link
Contributor

@anhldbk anhldbk commented Jul 22, 2021

Description

Checklist

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

  • Add tests for CLI (run command) to validate env variables
  • Modify code to pass the missing variables
  • Refactor code (run.go can be cleaner)
  • Update documentation: Env variables inconsistency docs#1835

@anhldbk anhldbk changed the title WIP: Pass more env variables to child processes WIP: Pass missing env variables to child processes Jul 22, 2021
@anhldbk anhldbk changed the title WIP: Pass missing env variables to child processes Pass missing env variables to child processes Jul 22, 2021
@anhldbk
Copy link
Contributor Author

anhldbk commented Jul 22, 2021

@yaron2 PTAL

If you don't see the refactor is necessary, I can revert it

@anhldbk
Copy link
Contributor Author

anhldbk commented Jul 23, 2021

@yaron2 I see the failed tests regarding to arguments app-port.

Question: if the port is not set, do we need to assign it with a free port? See line https://github.com/dapr/cli/blob/master/pkg/standalone/run.go#L103

@yaron2
Copy link
Member

yaron2 commented Jul 28, 2021

Tests are failing

@anhldbk
Copy link
Contributor Author

anhldbk commented Jul 28, 2021

Tests are failing

@yaron2 Pls answer my question above #770 (comment)

@anhldbk anhldbk requested review from a team as code owners July 30, 2021 14:25
@mukundansundar
Copy link
Collaborator

@yaron2 I see the failed tests regarding to arguments app-port.

Question: if the port is not set, do we need to assign it with a free port? See line https://github.com/dapr/cli/blob/master/pkg/standalone/run.go#L103

@anhldbk For this scenario no we should not be setting it using Freeport since then daprd will be waiting for the port to be active to communicate with the application. This port is used for daprd to app communication and if that is not needed and only the app communicates with dapr, the port will not be specified.

See the docs link for the --app-port field
This parameter tells Dapr which port your application is listening on.

@artursouza
Copy link
Contributor

What scenario is this change enabling? Basically trying to understand why we need to set those via env variables.

@anhldbk
Copy link
Contributor Author

anhldbk commented Aug 4, 2021

@artursouza I noted on the description above.

Python SDK has a different view on such variables. See global_settings.py. This leads to not-working tutorial

The root cause is: dapr cli doesn't pass necessary environment variables to child processes.

So is it good to make a PR?

@mukundansundar
Copy link
Collaborator

@artursouza I noted on the description above.

Python SDK has a different view on such variables. See global_settings.py. This leads to not-working tutorial

The root cause is: dapr cli doesn't pass necessary environment variables to child processes.

So is it good to make a PR?

@anhldbk The specific settings that are in global_settings.py are specific to python SDK. They will be processed by the SDK and not by the CLI directly.
@wcs1only Thoughts?

For the tutorial linked, I am able to run it using dapr CLI and it is working, there is a slight change in the docs that is needed, it is currently having the curl commands as

curl http://localhost:3500/v1.0/invoke/cart/method/add -X POST

While that is true in general cases, the dapr run command is as follows

dapr run --app-id cart --app-port 5000 python app.py

Here the argument for run setting the HTTP PORT for Dapr is not given, hence a random port is selected example:

$ dapr run --app-id cart --app-port 5000 python app.py
ℹ️  Starting Dapr with id cart. HTTP Port: 55738. gRPC Port: 55739

The tutorial then says run

curl http://localhost:3500/v1.0/invoke/cart/method/add -X POST
curl: (7) Failed to connect to localhost port 3500: Connection refused

which fails with the above failure.

But if the CLI is used to invoke the method as shown in the CLI tab in the tutorial,

$ dapr invoke --app-id cart --method add
Added!

it works as expected.

if it is run with

dapr run --app-id cart --app-port 5000 -H 3500 python app.py
ℹ️  Starting Dapr with id cart. HTTP Port: 3500. gRPC Port: 55798

Then the HTTP port is also specified and the curl command also works:

$ curl http://localhost:3500/v1.0/invoke/cart/method/add -X POST
Added!

I think this is a change that is needed in the docs, specifying the -H or --dapr-http-port argument for dapr run command to fix the HTTP port as 3500 for the following curl command in the tutorial .
@artursouza @wcs1only thoughts on this?

@anhldbk
Copy link
Contributor Author

anhldbk commented Aug 5, 2021

@mukundansundar I don't think it's specific only to Python SDK.

The problem here is:

  • There's an inconsistency in code & docs in Env variables
  • The inconsistency leads to wrongly SDK implementations

@yaron2 What do you think? Please comment so I know how to proceed.

@yaron2
Copy link
Member

yaron2 commented Aug 9, 2021

@mukundansundar I don't think it's specific only to Python SDK.

The problem here is:

  • There's an inconsistency in code & docs in Env variables
  • The inconsistency leads to wrongly SDK implementations

@yaron2 What do you think? Please comment so I know how to proceed.

@anhldbk you raise a real issue which is that there are inconsistencies. I think as step 1 we need to document the inconsistencies and then fix them in documentation. If you're willing to help out with this since you seem to have a good knowledge of this, then that'd be great and highly appreciated.

After we have a list of inconsistencies we can decide how to tackle it. In general, there are SDK specific env vars that shouldn't be passed to the child app. So I think mapping these out are a must before we continue looking at this PR.

@anhldbk
Copy link
Contributor Author

anhldbk commented Aug 10, 2021

@yaron2

If you're willing to help out with this since you seem to have a good knowledge of this, then that'd be great and highly appreciated.

Yes! Gonna work on dapr/docs. Please keep this PR ;)

@anhldbk anhldbk changed the title Pass missing env variables to child processes WIP: Pass missing env variables to child processes Sep 18, 2021
@anhldbk
Copy link
Contributor Author

anhldbk commented Oct 23, 2021

@yaron2 Me again. I updated the docs via: dapr/docs#1835

Gonna improve this PR

daixiang0 and others added 24 commits December 22, 2021 14:10
* fix shutdown hang when enable socket

Signed-off-by: Loong <loong.dai@intel.com>

* feedback

Signed-off-by: Loong <loong.dai@intel.com>

Co-authored-by: Loong <loong.dai@intel.com>
Co-authored-by: Mukundan Sundararajan <musundar@microsoft.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
* Run tests against new hotfix releases

* Update e2e test for latest runtime releases

Signed-off-by: Andy Le <anhldbk@gmail.com>
Co-authored-by: Mukundan Sundararajan <musundar@microsoft.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@gmail.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
This reverts commit 7d02ef6.

Signed-off-by: Andy Le <anhldbk@gmail.com>
This reverts commit 73a414a.

Signed-off-by: Andy Le <anhldbk@gmail.com>
CLI
- Remove arg `place-host-address`

Application
- Remove env `DAPR_PROFILE_PORT`, `HOST_ADDRESS`
- Pass env `APP_PORT` conditionally (if set)
* runtime: support unix domain socket

Signed-off-by: Long <long.dai@intel.com>

* feedback

Signed-off-by: Long <long.dai@intel.com>

* update example

Signed-off-by: Long <long.dai@intel.com>
Signed-off-by: Long <long0dai@foxmail.com>
Signed-off-by: Long <long.dai@intel.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
* Fix upgrade paths to 1.5.0 runtime

* Update tests/e2e/standalone/standalone_test.go

* Update tests/e2e/standalone/standalone_test.go

* Update tests/e2e/standalone/standalone_test.go

* Update tests/e2e/standalone/standalone_test.go

Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
* Publish events with CloudEvent envelope with content type application/cloudevents+json

* fix linter issue

* Adding e2e test

* Tweak
* fix shutdown hang when enable socket

Signed-off-by: Loong <loong.dai@intel.com>

* feedback

Signed-off-by: Loong <loong.dai@intel.com>

Co-authored-by: Loong <loong.dai@intel.com>
Co-authored-by: Mukundan Sundararajan <musundar@microsoft.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
Signed-off-by: Andy Le <anhldbk@gmail.com>
anhldbk added a commit to anhldbk/dapr-cli that referenced this pull request Dec 22, 2021
- In the past, I openned this PR: dapr#770
- DCO had some complaints.
- I incorrectly rebased.
- This PR is made by forking the master branch, modifying related files
@anhldbk anhldbk closed this Dec 22, 2021
@anhldbk anhldbk mentioned this pull request Dec 22, 2021
4 tasks
anhldbk added a commit to anhldbk/dapr-cli that referenced this pull request Dec 22, 2021
- In the past, I openned this PR: dapr#770
- DCO had some complaints.
- I incorrectly rebased.
- This PR is made by forking the master branch, modifying related files

Signed-off-by: Andy Le <anhldbk@gmail.com>
yaron2 pushed a commit that referenced this pull request Jan 3, 2022
- In the past, I openned this PR: #770
- DCO had some complaints.
- I incorrectly rebased.
- This PR is made by forking the master branch, modifying related files

Signed-off-by: Andy Le <anhldbk@gmail.com>
imneov pushed a commit to imneov/dapr-cli that referenced this pull request Mar 18, 2022
- In the past, I openned this PR: dapr#770
- DCO had some complaints.
- I incorrectly rebased.
- This PR is made by forking the master branch, modifying related files

Signed-off-by: Andy Le <anhldbk@gmail.com>
Signed-off-by: imneov <grantliu@yunify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.