Add GatewayPort configuration #67

jonatasbaldin · 2019-09-07T06:10:47Z

Description

Allows the User to configure a custom GatewayPort in nats-queue-worker. Fixes #59.

Motivation and Context

I have raised an issue to propose this change (required)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

acornies

This looks good to me.

alexellis

Sorry, just noticed these comments were pending and not sent yet.

alexellis · 2019-09-07T07:37:22Z

readconfig.go

@@ -114,6 +120,7 @@ func (ReadConfig) Read() QueueWorkerConfig {
 type QueueWorkerConfig struct {
 	NatsAddress    string
 	GatewayAddress string
+	GatewayPort    string


Please read as an int

alexellis · 2019-09-07T07:38:52Z

readconfig.go

+		cfg.GatewayPort = val
+	} else {
+		cfg.GatewayPort = "8080"
+	}


We could probably concatenate the value once here instead of passing two arguments everywhere.

I think the new env var makes sense to prevent issues upgrading versions.

acornies · 2019-09-11T15:25:38Z

main.go

 		pathVal,
 		qs)

 	if config.GatewayInvoke {
-		functionURL = fmt.Sprintf("http://%s:8080/function/%s%s%s",
+		functionURL = fmt.Sprintf("http://%s:%s/function/%s%s%s",


We should probably make the entire url configurable and do something like url.Parse(). Opens the door for https etc.

That's nice!

We could add yet a new variable called cfg.GatewayProtocol with http as the default value for backwards compatibility. What do you think?

Also, should we open a new issue/PR for that?

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

jonatasbaldin · 2019-09-11T16:26:43Z

I closed the PR by mistake 🤦‍♂ @alexellis, could you re run the build, please?

Also, you'll note that since I'm concatenating the cfg.GatewayAddress and cfg.GatewayPort I needed to change the test_gatewayaddr test to accommodate the port as well. It looks a bit odd, but it's working.

jonatasbaldin · 2019-10-01T01:24:32Z

hey @alexellis, any comments on this? ✨ could u run the build again? hehe 🏃

acornies · 2019-10-07T20:04:49Z

@alexellis please advise

alexellis · 2019-10-07T20:14:03Z

Retrying the build.

alexellis · 2019-10-07T20:14:40Z

main.go

@@ -30,14 +30,15 @@ func makeFunctionURL(req *queue.Request, config *QueueWorkerConfig, path, queryS
 	if len(path) > 0 {
 		pathVal = path
 	}
-	functionURL := fmt.Sprintf("http://%s%s:8080%s%s",
+	functionURL := fmt.Sprintf("http://%s%s:%d%s%s",


This has been changed incorrectly.

This line does not invoke via the gateway.

@acornies FYI ^

alexellis · 2019-10-07T20:15:38Z

readconfig_test.go

@@ -76,12 +77,18 @@ func Test_ReadConfig(t *testing.T) {
 		t.Fail()
 	}

-	expected = "test_gatewayaddr"
+	expected = "test_gatewayaddr:8080"


Can you use the word want instead of expected and gotinstead ofactual`? Thank you, see: https://blog.alexellis.io/golang-writing-unit-tests/

I think we should then refactor the whole test file, since all tests are using expected and actual and we should do this in another PR. Do you agree?

Yes, but this is not refactoring, it's naming. Please go ahead.

alexellis

Found a potential bug in the implementation, please see comment.

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

jonatasbaldin

Comments answered 🎉

jonatasbaldin · 2019-10-08T23:40:10Z

main.go

@@ -30,14 +30,15 @@ func makeFunctionURL(req *queue.Request, config *QueueWorkerConfig, path, queryS
 	if len(path) > 0 {
 		pathVal = path
 	}
-	functionURL := fmt.Sprintf("http://%s%s:8080%s%s",
+	functionURL := fmt.Sprintf("http://%s%s:%d%s%s",


jonatasbaldin · 2019-10-08T23:42:55Z

readconfig_test.go

@@ -76,12 +77,18 @@ func Test_ReadConfig(t *testing.T) {
 		t.Fail()
 	}

-	expected = "test_gatewayaddr"
+	expected = "test_gatewayaddr:8080"


I think we should then refactor the whole test file, since all tests are using expected and actual and we should do this in another PR. Do you agree?

alexellis

Sending comments from mobile

alexellis · 2019-10-09T06:50:56Z

readconfig_test.go

 	if config.GatewayAddress != expected {
 		t.Logf("Expected GatewayAddress `%s` actual `%s`\n", expected, config.GatewayAddress)
 		t.Fail()
 	}

+	expectedGatewayPort := 8080
+	if config.GatewayPort != expectedGatewayPort {
+		t.Logf("Expected GatewayPort `%d` actual `%d`\n", expectedGatewayPort, config.GatewayPort)


I.e. GatewayPort want %d, got %d

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

alexellis

LGTM

Jonatas Baldin added 3 commits September 7, 2019 08:07

Add faas_gateway_port env var and GatewayAddress config

6ea3fb3

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

Use GatewayAddress config when creating the gateway URL

86009b9

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

Add faas_gateway_port in README

f129046

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

derek bot added the new-contributor label Sep 7, 2019

acornies approved these changes Sep 9, 2019

View reviewed changes

alexellis requested changes Sep 11, 2019

View reviewed changes

acornies reviewed Sep 11, 2019

View reviewed changes

Jonatas Baldin added 2 commits September 11, 2019 18:19

Change GatewayPort to int and concatenate it with GatewayAddress

02a9466

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

Fix test_gatewayaddr test

5cd92a1

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

jonatasbaldin closed this Sep 11, 2019

jonatasbaldin reopened this Sep 11, 2019

alexellis reviewed Oct 7, 2019

View reviewed changes

alexellis requested changes Oct 7, 2019

View reviewed changes

Remove GatewayPort from functionURL

0b02d9f

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

jonatasbaldin commented Oct 8, 2019

View reviewed changes

alexellis reviewed Oct 9, 2019

View reviewed changes

Jonatas Baldin added 2 commits October 10, 2019 07:19

Rename expected to want and instead to go in tests

0b8dcbd

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

Rephrase test messages

1d79dac

Signed-off-by: Jonatas Baldin <jonatas.baldin@gmail.com>

alexellis approved these changes Oct 11, 2019

View reviewed changes

alexellis merged commit d35631d into openfaas:master Oct 11, 2019

alexellis mentioned this pull request Nov 25, 2019

Fix for issue #73 #74

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GatewayPort configuration #67

Add GatewayPort configuration #67

jonatasbaldin commented Sep 7, 2019

acornies left a comment

alexellis left a comment

alexellis Sep 7, 2019

acornies Sep 11, 2019

jonatasbaldin Sep 11, 2019

alexellis Sep 7, 2019

jonatasbaldin Sep 11, 2019

acornies Sep 11, 2019

jonatasbaldin Sep 11, 2019 •

edited

jonatasbaldin commented Sep 11, 2019

jonatasbaldin commented Oct 1, 2019

acornies commented Oct 7, 2019

alexellis commented Oct 7, 2019

alexellis Oct 7, 2019

alexellis Oct 7, 2019

jonatasbaldin Oct 8, 2019

alexellis Oct 7, 2019

jonatasbaldin Oct 8, 2019

alexellis Oct 9, 2019

alexellis left a comment

jonatasbaldin left a comment

jonatasbaldin Oct 8, 2019

jonatasbaldin Oct 8, 2019

alexellis left a comment

alexellis Oct 9, 2019

alexellis left a comment

Add GatewayPort configuration #67

Add GatewayPort configuration #67

Conversation

jonatasbaldin commented Sep 7, 2019

Description

Motivation and Context

Types of changes

Checklist:

acornies left a comment

Choose a reason for hiding this comment

alexellis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatasbaldin Sep 11, 2019 • edited

Choose a reason for hiding this comment

jonatasbaldin commented Sep 11, 2019

jonatasbaldin commented Oct 1, 2019

acornies commented Oct 7, 2019

alexellis commented Oct 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis left a comment

Choose a reason for hiding this comment

jonatasbaldin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis left a comment

Choose a reason for hiding this comment

jonatasbaldin Sep 11, 2019 •

edited