Use `LINK_PORT` variable if it's set #1370

mwarkentin · 2016-11-03T19:21:00Z

This brings PORT up to parity with SCHEME, PATH, USERNAME, and PASSWORD.

Use Case

We are trying to use the official rabbitmq image for local development.

That image EXPOSEs multiple ports
Convox uses the first one exposed in docker inspect
It doesn't provide a way to set it explicitly like the other LINK_* environment variables can be used

TODO

Update docs: https://github.com/convox/site/pull/340

This brings PORT up to parity with SCHEME, PATH, USERNAME, and PASSWORD.

codecov-io · 2016-11-03T19:26:07Z

Current coverage is 29.05% (diff: 0.00%)

Merging #1370 into master will decrease coverage by 0.02%

@@             master      #1370   diff @@
==========================================
  Files           137        137          
  Lines         12900      12913    +13   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           3752       3752          
- Misses         8750       8763    +13   
  Partials        398        398

Powered by Codecov. Last update 46de994...33567ef

mwarkentin · 2016-11-03T19:29:11Z

Came across these LINK_* references as well - not sure how they fit into things yet:

mwarkentin · 2016-11-03T20:38:27Z

My main question is around what needs to be done in release.go.

Do we need to support this both locally and on the rack? My use case is currently local only.
Do we need to support both public ('8000:8000) and private (8000) ports?
If so, would it make sense to write a function in ports.go which parses a port string and returns a Port instance?

mwarkentin · 2016-11-04T12:13:08Z

Confirmed that this change works locally:

docker-compose.yml:

worker:
  build: .
  command: /app/bin/worker
  environment:
    - ...
  links:
    - broker

broker:
  image: rabbitmq:3.6
  environment:
    - LINK_SCHEME=amqp
    - LINK_PORT=5672
  ports:
    - 5672

/Users/michaelwarkentin/go/src/github.com/convox/rack/cmd/convox/convox start

...

worker   │ running: docker run -i --rm --name notifications-worker ... --add-host broker:172.17.0.2 -e BROKER_SCHEME=amqp -e BROKER_HOST=172.17.0.2 -e BROKER_PORT=5672 -e BROKER_PATH= -e BROKER_USERNAME= -e BROKER_PASSWORD= -e BROKER_URL=amqp://172.17.0.2:5672 --add-host redis:172.17.0.4 ... notifications/worker sh -c /app/bin/worker

ddollar · 2016-11-07T18:47:34Z

Seems good. Yes, we should do it for production also, that would be here:

https://github.com/convox/rack/blob/master/api/models/release.go#L489

mwarkentin · 2016-11-07T19:06:57Z

@ddollar Thanks. I took a look at adding it there, but wasn't sure what to do for converting an environment variable (string) into a Port object: https://github.com/convox/rack/blob/master/manifest/ports.go#L5-L10

It's a little less clear to me how this would be used on a rack (as I'd expect the only option is to go through the service's ELB, which already has the exposed ports defined.

ddollar · 2016-11-07T19:07:54Z

I'm assuming this LINK_PORT defines which port shows up by default in the full link _URL ?

ddollar · 2016-11-07T19:09:32Z

Perhaps loop through other.Ports looking for the one where LINK_PORT matches .Container

mwarkentin · 2016-11-07T19:15:33Z

@ddollar I'll give that a shot, thanks!

mwarkentin · 2016-11-08T13:19:58Z

api/models/release.go

+				i, _ := strconv.Atoi(other.Exports["LINK_PORT"])
+				if err != nil {
+					// handle error
+					fmt.Println(err)


Golang noobie.. not sure what to do here in case of an error converting LINK_PORT to an int.

return nil, err

mwarkentin · 2016-11-08T13:20:34Z

api/models/release.go

+					if i == p.Container {
+						port = p
+					}
+				}


Anything we should do if they've set LINK_PORT but it doesn't match any of the exposed ports?

That should be an error.

* If `LINK_PORT` not set, default to first port immediately * Return error if `LINK_PORT` can't be converted to integer * Return error if `LINK_PORT` set to non-existant port

mwarkentin · 2016-11-10T19:38:57Z

api/models/release.go

@@ -18,7 +19,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/iam"
 	"github.com/convox/rack/api/crypt"
 	"github.com/convox/rack/api/structs"
-	"github.com/convox/rack/manifest"
+	mp "github.com/convox/rack/manifest"


Had to do this as I wasn't able to refer to manifest.Port in my code otherwise (there's a local variable also called manifest already). Open on suggestions for a better way to handle this (or better name for it - mp stands for manifest package).

I updated other the other references that I found as well.

So let's go the other way with this to avoid surprises around the namespace. Perhaps name the variable m down in the function and let this be manifest

Can do, thanks!

ddollar · 2016-11-10T19:46:25Z

api/models/release.go

@@ -18,7 +19,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/iam"
 	"github.com/convox/rack/api/crypt"
 	"github.com/convox/rack/api/structs"
-	"github.com/convox/rack/manifest"
+	mp "github.com/convox/rack/manifest"


So let's go the other way with this to avoid surprises around the namespace. Perhaps name the variable m down in the function and let this be manifest

* Imported package is back to `manifest` * Manifest arg to `resolveLinks` is now `m` * Pointer to manifest is now `mp`

mwarkentin · 2016-11-10T19:55:11Z

api/models/release.go

-func (r *Release) resolveLinks(app App, manifest *manifest.Manifest) (*manifest.Manifest, error) {
-	m := *manifest
+func (r *Release) resolveLinks(app App, m *manifest.Manifest) (*manifest.Manifest, error) {
+	mp := *m


Ran into one more existing variable name conflict here - renamed the pointer to the manifest from m to mp and updated other references.

mwarkentin · 2016-11-10T20:34:30Z

Updated docs here: https://github.com/convox/site/pull/340

mwarkentin · 2016-11-11T14:08:21Z

@MiguelMoll Not sure if you saw this one, @ddollar mentioned that it could make it into the next release potentially?

MiguelMoll · 2016-11-16T17:37:23Z

@mwarkentin Getting the release ready 👍 Could you update this branch with master? Then we'll be ready to go.

mwarkentin · 2016-11-16T18:48:56Z

@MiguelMoll Updated, thanks!

mwarkentin · 2016-11-18T13:38:28Z

@MiguelMoll Thanks!

Use LINK_PORT variable if it's set

45a8bfb

This brings PORT up to parity with SCHEME, PATH, USERNAME, and PASSWORD.

Update fixtures

cb4360a

Loop through ports to match LINK_PORT

b60688e

mwarkentin commented Nov 8, 2016

View reviewed changes

Improve port matching

dc994f6

* If `LINK_PORT` not set, default to first port immediately * Return error if `LINK_PORT` can't be converted to integer * Return error if `LINK_PORT` set to non-existant port

mwarkentin commented Nov 10, 2016

View reviewed changes

ddollar suggested changes Nov 10, 2016

View reviewed changes

Rename import and variables

90f230b

* Imported package is back to `manifest` * Manifest arg to `resolveLinks` is now `m` * Pointer to manifest is now `mp`

mwarkentin commented Nov 10, 2016

View reviewed changes

Remove manifest pointer

97f740f

ddollar approved these changes Nov 10, 2016

View reviewed changes

ddollar added the merge label Nov 14, 2016

MiguelMoll added this to the 20161116 milestone Nov 16, 2016

Merge branch 'master' into link-port-support

33567ef

MiguelMoll added a commit that referenced this pull request Nov 16, 2016

[merge] pull request #1370

6cbad41

MiguelMoll mentioned this pull request Nov 16, 2016

[RELEASE] 20161116 #1400

Merged

18 tasks

MiguelMoll added a commit that referenced this pull request Nov 16, 2016

[merge] pull request #1370

596ae60

MiguelMoll added a commit that referenced this pull request Nov 17, 2016

[merge] pull request #1370

c5acf31

MiguelMoll closed this in #1400 Nov 18, 2016

mwarkentin deleted the link-port-support branch November 18, 2016 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `LINK_PORT` variable if it's set #1370

Use `LINK_PORT` variable if it's set #1370

mwarkentin commented Nov 3, 2016 •

edited

codecov-io commented Nov 3, 2016 •

edited

mwarkentin commented Nov 3, 2016

mwarkentin commented Nov 3, 2016

mwarkentin commented Nov 4, 2016

ddollar commented Nov 7, 2016

mwarkentin commented Nov 7, 2016

ddollar commented Nov 7, 2016

ddollar commented Nov 7, 2016

mwarkentin commented Nov 7, 2016

mwarkentin Nov 8, 2016

ddollar Nov 10, 2016

mwarkentin Nov 8, 2016

ddollar Nov 10, 2016

mwarkentin Nov 10, 2016 •

edited

ddollar Nov 10, 2016

mwarkentin Nov 10, 2016

ddollar Nov 10, 2016

mwarkentin Nov 10, 2016 •

edited

mwarkentin commented Nov 10, 2016

mwarkentin commented Nov 11, 2016

MiguelMoll commented Nov 16, 2016

mwarkentin commented Nov 16, 2016

mwarkentin commented Nov 18, 2016

Use LINK_PORT variable if it's set #1370

Use LINK_PORT variable if it's set #1370

Conversation

mwarkentin commented Nov 3, 2016 • edited

Use Case

TODO

codecov-io commented Nov 3, 2016 • edited

Current coverage is 29.05% (diff: 0.00%)

mwarkentin commented Nov 3, 2016

mwarkentin commented Nov 3, 2016

mwarkentin commented Nov 4, 2016

ddollar commented Nov 7, 2016

mwarkentin commented Nov 7, 2016

ddollar commented Nov 7, 2016

ddollar commented Nov 7, 2016

mwarkentin commented Nov 7, 2016

mwarkentin Nov 8, 2016

Choose a reason for hiding this comment

ddollar Nov 10, 2016

Choose a reason for hiding this comment

mwarkentin Nov 8, 2016

Choose a reason for hiding this comment

ddollar Nov 10, 2016

Choose a reason for hiding this comment

mwarkentin Nov 10, 2016 • edited

Choose a reason for hiding this comment

ddollar Nov 10, 2016

Choose a reason for hiding this comment

mwarkentin Nov 10, 2016

Choose a reason for hiding this comment

ddollar Nov 10, 2016

Choose a reason for hiding this comment

mwarkentin Nov 10, 2016 • edited

Choose a reason for hiding this comment

mwarkentin commented Nov 10, 2016

mwarkentin commented Nov 11, 2016

MiguelMoll commented Nov 16, 2016

mwarkentin commented Nov 16, 2016

mwarkentin commented Nov 18, 2016

Use `LINK_PORT` variable if it's set #1370

Use `LINK_PORT` variable if it's set #1370

mwarkentin commented Nov 3, 2016 •

edited

codecov-io commented Nov 3, 2016 •

edited

mwarkentin Nov 10, 2016 •

edited

mwarkentin Nov 10, 2016 •

edited