Skip to content
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

Improve hydra initialization process #128

Merged
merged 4 commits into from
May 24, 2017
Merged

Improve hydra initialization process #128

merged 4 commits into from
May 24, 2017

Conversation

cjus
Copy link
Contributor

@cjus cjus commented May 18, 2017

This PR seeks to improve the initialization process by offering an additional way of initializing hydra beyond the use of a configuration file and object.

The init function currently accepts a config parameter which can be an object or string. If it's a string then the string is expected to point to a file path for a configuration file. If the config parameter is an object then hydra will attempt to use the object members for initialization.

This PR adds the possibility of passing a config parameter of null or undefined. Doing so will initiate the following flow:

  • If the config is null then a new config is created with default values.
  • Redis will then be expected to exists locally unless specified.
  • A check is made for process.env.HYDRA_REDIS_URL. If defined then the config redis.url will point to the string specified.
  • Next, process.env.HYDRA_SERVICE is checked. If that environment variable exists then it can potentially hold one of two types of formatted strings.
"hello-service:1.2.3"

or:

"serviceName=hello-service|serviceDescription=A service that says hello|serviceIP=127.0.0.1|servicePort=5000|serviceType=greeter"

The first format is what we already use when working with Docker containers. The second format represents a string containing pipe delimited | key/value pairs. The key/value pairs are separated by the = symbol.

The string is parsed and an internal config object is created.

This allows a service to be started at the command line using:

$ HYDRA_SERVICE="serviceName=hello-service|serviceDescription=A service that says hello|serviceIP=127.0.0.1|servicePort=5000|serviceType=greeter|serviceVersion=0.1.3" HYDRA_REDIS_URL=redis://localhost:6379/15" node test.js

This allow allows for starting a Docker container and passing the environment variables:

$ sudo docker service create \
 --name hydra-router \
 --network servicenet \
 --restart-condition any \
 --restart-max-attempts 5 \
 --update-delay 10s \
 --constraint=node.role==manager \
 --env HYDRA_REDIS_URL="redis://10.0.0.154:6379/15" \
 --env HYDRA_SERVICE="serviceName=hydra-router|serviceDescription=A service-aware router|serviceIP=|servicePort=80|serviceType=router|serviceVersion1.3.11" HYDRA_REDIS_URL="redis://localhost:6379/15"" \
 --publish 80:80 \
 --replicas=3 \
 flywheelsports/hydra-router:1.3.11

A side effect of this PR is that creating test services locally does not require a config file. Only serviceName and servicePort need to be defined.

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. My one concern is using a simple delimiter-based format; what happens if someone needs a config entry with a | or = in it?

My proposal would be to allow it to contain JSON too. If the first character is a { it could attempt to parse JSON. That way if someone really needs to use a | or a = there'd still be a way to use this feature.

@cjus
Copy link
Contributor Author

cjus commented May 19, 2017

@emadum The service user will definitely need to ensure that they don't use | and = inside the values they provide. I think that's reasonable. I'll update this PR to support parsing JSON as well - that's a nice addition!

@cjus cjus merged commit b651028 into pnxtech:master May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants