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.

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

Remove invalid hostname characters from nats client id #45

Merged
merged 1 commit into from Jan 8, 2019

Conversation

Projects
None yet
4 participants
@ewilde
Copy link
Member

ewilde commented Oct 22, 2018

Todo

  • re-test

Similar to #24 running this docker image on an ecs instance.
ECS hostname contains . which is not supported by nats as a value for the clientid.

Description

When running this docker image on Amazon ECS you get the following error:

stan: invalid clientID: only alphanumeric and -or_ characters allowed.

This is because the ECS host name contains periods . i.e. ip-10-0-1-140.eu-west-1.compute.internal

How Has This Been Tested?

  • This has been tested on ECS in a custom build of nats-queue-worker and now starts properly

Test output:

Wait for 5m0s
Listening on [faas-request], clientID=[faas-worker-ip-10-0-4-89_eu-west-1_compute_internal], qgroup=[faas] durable=[]
  • Swarm

Test output:

Loading basic authentication credentials
Wait for  5m0s
Listening on [faas-request], clientID=[faas-worker-2b97ea65f696], qgroup=[faas] durable=[]

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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. Didn't see any tests in the project, happy to add if that's what's wanted
  • All new and existing tests passed.

@ewilde ewilde changed the title WIP - Remove invalid hostname characters from nats client id Remove invalid hostname characters from nats client id Oct 22, 2018

@rgee0
Copy link
Member

rgee0 left a comment

Looks good, Ed.

Just one question around whether the regexp definition ought to be MustCompile particularly since we aren't using the error out of Compile?

@ewilde ewilde force-pushed the ewilde:error-logging branch from 761c0a2 to 94c6296 Oct 27, 2018

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Nov 3, 2018

There's outstanding feedback, so we want to fix it before merging? @rgee0 @ewilde

@ewilde ewilde changed the title Remove invalid hostname characters from nats client id [WIP] Remove invalid hostname characters from nats client id Nov 14, 2018

Show resolved Hide resolved Dockerfile Outdated

@ewilde ewilde force-pushed the ewilde:error-logging branch from eebdd77 to 3472f31 Nov 15, 2018

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Dec 16, 2018

@rgee0 and @alexellis is there anything else to do here?

@ewilde ewilde force-pushed the ewilde:error-logging branch 3 times, most recently from 6424f2a to 4b37b3d Dec 16, 2018

@ewilde

This comment has been minimized.

Copy link
Member

ewilde commented Dec 16, 2018

I just need to re-test since I addressed comments

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Dec 29, 2018

@ewilde please can you give an update?

I'm not sure if I can merge this since "todo" says "re-test" and the title says "WIP".

Alex

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Dec 29, 2018

Derek add label: blocked

@derek derek bot added the blocked label Dec 29, 2018

Remove invalid hostname characters from nats client id
Signed-off-by: Edward Wilde <ewilde@gmail.com>

@ewilde ewilde force-pushed the ewilde:error-logging branch from 4b37b3d to 38727e9 Dec 31, 2018

@ewilde ewilde changed the title [WIP] Remove invalid hostname characters from nats client id Remove invalid hostname characters from nats client id Dec 31, 2018

@ewilde

This comment has been minimized.

Copy link
Member

ewilde commented Dec 31, 2018

@alexellis PTAL, retested on swarm and fargate

@alexellis
Copy link
Member

alexellis left a comment

LGTM

@alexellis alexellis merged commit 4d38388 into openfaas:master Jan 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment