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.

Sign up for GitHub

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

Jump to bottom

-app-port for listen port, mm installation #659

Merged

msfussell merged 1 commit into dapr:master from akhilac1:pubSub_go_quickstart_appPort

Apr 29, 2022

Contributor

akhilac1 commented Apr 25, 2022

Update order_processor to listen on -app-port vs. hardcoded port number of 6001. This hardwiring requires the documentation to be updated every time the port changes. Currently, the port used and the documentation are out of sync. By moving to listening on -app-port, order-processor can be run successfully irrespective of the port specified by the user.
Since quickstarts are for folks bootstrapping on Dapr it is good to automate dependency setup. To run make in quickstarts, mechanical_markdown is a hard dependency. This is not clearly documented and hence make fails. Updated validate.mk to install mechanical_markdown in case it is not already installed.

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: #656

##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.

shubham1172 reviewed

View reviewed changes

validate.mk Outdated

@@ @@ -2,4 +2,5 @@ @@
               MM_SHELL ?= bash -c
               validate:
+              	type mm.py || sudo pip install mechanical_markdown

Member

shubham1172 Apr 25, 2022

Probably move this to a new make-command, like ensure-mm - that installs mechanical markdown if needed.

shubham1172 reviewed

View reviewed changes

validate.mk Outdated

@@ @@ -2,4 +2,5 @@ @@
               MM_SHELL ?= bash -c
               validate:
+              	type mm.py || sudo pip install mechanical_markdown

Member

shubham1172 Apr 25, 2022

Typo: mechanical-markdown

shubham1172 reviewed

View reviewed changes

pub_sub/go/sdk/order-processor/app.go Outdated

@@ @@ -18,7 +19,13 @@ var sub = &common.Subscription{ @@
               }
               func main() {
-              	s := daprd.NewService(":6001")
+              	//read app-port passed thru command line

Member

shubham1172 Apr 25, 2022

Suggested change

      
            	//read app-port passed thru command line
          
            	// read the app-port from the environment variable

However, it seems unnecessary.

shubham1172 reviewed

View reviewed changes

pub_sub/go/sdk/order-processor/app.go

@@ @@ -18,7 +19,13 @@ var sub = &common.Subscription{ @@
               }
               func main() {
-              	s := daprd.NewService(":6001")
+              	//read app-port passed thru command line
+              	appPort, isSet := os.LookupEnv("APP_PORT")

Member

shubham1172 Apr 25, 2022

Suggested change

      
            	appPort, isSet := os.LookupEnv("APP_PORT")
          
            	if appPort, ok := os.LookupEnv("APP_PORT"); !ok {
          
            	        // fail
          
            	}

Contributor Author

akhilac1 Apr 26, 2022

@shubham1172 the intent is to initialize and declare the variables based on os.Lookup. Including them in 'if' would require a declaration of appPort since it is used outside the if condition. I am not in favour of adding an else condition since it impacts readability of the simple flow. Will leave this as-is.

shubham1172 reviewed

View reviewed changes

pub_sub/go/sdk/order-processor/app.go Outdated

+              	//read app-port passed thru command line
+              	appPort, isSet := os.LookupEnv("APP_PORT")
+              	if !isSet {
+              		log.Fatalf("--app-port is not set")

Member

shubham1172 Apr 25, 2022

The error message should mention that the environment variable was not set. It is set by passing the --app-port option to Dapr, but this message can be misleading for users. We can additionally add a hint to the error message that this environment variable is set by Dapr. Thoughts?

Member

shubham1172 commented Apr 25, 2022

@akhilac1 please fix DCO https://docs.dapr.io/contributing/contributing-overview/#developer-certificate-of-origin-signing-your-work

surenderssm reviewed

View reviewed changes

pub_sub/go/sdk/order-processor/app.go

@@ @@ -18,7 +19,13 @@ var sub = &common.Subscription{ @@
               }
               func main() {
-              	s := daprd.NewService(":6001")
+              	//read app-port passed thru command line
+              	appPort, isSet := os.LookupEnv("APP_PORT")

surenderssm Apr 25, 2022

How an argument in daprd gets linked to environment variable?

Member

shubham1172 Apr 25, 2022

https://github.com/dapr/dapr/blob/aa6a9092e67bf50ca921a4f443e00e9afa468b04/pkg/runtime/cli.go#L213-L226

Contributor Author

akhilac1 Apr 26, 2022

Thanks for this question @surenderssm. I have added comment in the code to explore the dapr command line options and corresponding environment variables. It makes the quickstart code more comprehensible.

shubham1172 reviewed

View reviewed changes

pub_sub/go/sdk/order-processor/app.go Outdated Show resolved Hide resolved

Member

yaron2 commented Apr 26, 2022

Member

msfussell commented Apr 27, 2022

This change applies universally across all the quickstarts. At least this PR should also apply to Go examples for HTTP and SDK for Pub/Sub, Service Invocation and State Management. This only fixes Pub/Sub currently.

mukundansundar reviewed

View reviewed changes

validate.mk

               validate:
               	mm.py -l -s "${MM_SHELL}" README.md
+              install_mm:

Contributor

mukundansundar Apr 27, 2022

change ideally not needed ... see the validate.yaml workflow ... @akhilac1

akhilac1 force-pushed the pubSub_go_quickstart_appPort branch 2 times, most recently from c1c0308 to 9688c6a Compare

April 27, 2022 11:11

Contributor

amulyavarote commented Apr 27, 2022

@akhilac1 Can you rebase this PR with master and push the changes for the builds to be successful?

akhilac1 force-pushed the pubSub_go_quickstart_appPort branch from 9688c6a to a3fde65 Compare

April 28, 2022 05:04


          -app-port for listen port, mm installation

22a3dc3

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

Resolve PR comments

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

Update pub_sub/go/sdk/order-processor/app.go

Co-authored-by: Shubham Sharma <shubhash@microsoft.com>
Signed-off-by: akhilac1 <chetlapalle.akhila@gmail.com>

akhilac1 force-pushed the pubSub_go_quickstart_appPort branch from a3fde65 to 22a3dc3 Compare

April 29, 2022 06:27

Contributor Author

akhilac1 commented Apr 29, 2022 •

edited

@amulyavarote - The tests are passing now. Can you please approve the PR so that merge can be completed. Thank you!!

Contributor Author

akhilac1 commented Apr 29, 2022

This change applies universally across all the quickstarts. At least this PR should also apply to Go examples for HTTP and SDK for Pub/Sub, Service Invocation and State Management. This only fixes Pub/Sub currently.

@msfussell - Will cover this in #661

mukundansundar approved these changes

View reviewed changes

Contributor

mukundansundar left a comment

lgtm

shubham1172 approved these changes

View reviewed changes

amulyavarote approved these changes

View reviewed changes

msfussell merged commit 017da7d into dapr:master

yaron2 assigned akhilac1

yaron2 added this to the 1.8 milestone

akhilac1 mentioned this pull request

Dapr Go Pub Sub Quick Start - Doc update and graceful shutdown #656

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment