jooby consul module #740
jooby consul module #740
Conversation
jknack
left a comment
jknack
left a comment
There was a problem hiding this comment.
Looks good, I made a couple of comments, but also:
- add jooby-consul to modules section in parent pom
- add jooby-consul to dependency-management section in parent pom
- add a unit test. Tests must cover all the possible branches
jooby-consul/README.md
Outdated
| @@ -0,0 +1,53 @@ | |||
| # consul | |||
There was a problem hiding this comment.
move the file into md/doc/consul/consul.md
jooby-consul/README.md
Outdated
| <dependency> | ||
| <groupId>org.jooby</groupId> | ||
| <artifactId>jooby-consul</artifactId> | ||
| <version>1.1.1-SNAPSHOT</version> |
There was a problem hiding this comment.
replace version with: <version>{{version}}</version>
jooby-consul/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.orbitz.consul</groupId> | ||
| <artifactId>consul-client</artifactId> | ||
| <version>0.14.0</version> |
There was a problem hiding this comment.
create a <consul.version>0.14.0</consul.version> property in parent pom and use it here.
| import java.util.UUID; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| public class ConsulModule implements Jooby.Module { |
There was a problem hiding this comment.
add javadoc like you did in consul.md
|
|
||
| private final String name; | ||
|
|
||
| public ConsulModule() { |
There was a problem hiding this comment.
add javadoc to default constructor
| this("default"); | ||
| } | ||
|
|
||
| public ConsulModule(String name) { |
There was a problem hiding this comment.
javadoc to specialized constructor
|
Thank you for your comments! I guess I need to add the module to the bom too? I never really understood this bom thing :) Unit test is already on the way. |
|
I'm impressed on how you find things that are not documented (like the bom or the startAsync in your previous pull) well done! Bom is auto-generated with Coverage? It is bit more tricky. There is a script to launch: |
|
I would like to release 1.1.1 later today. Should I wait for consul module? Thanks |
|
Oh, I guess it is ready, code wise. I can write the javadoc first. Anyway, do not compromise your 1.1.1 release date because of this module. |
pierredavidbelanger
force-pushed
the
jooby-consul
branch
from
3a0dd71 to
ee08080
Compare
|
I guess everything you asked is now commit and pushed to this PR. If you want to include this module into your 1.1.1 release, I can do a rebase and cleanup the commit history today before you have a look at the finished product and merge it. |
jknack
left a comment
jknack
left a comment
There was a problem hiding this comment.
Can you rename ConsulModule to Consul?
You will need to update the doc too
|
Sure I can, so, either: use(new org.jooby.consul.Consul());or get("/", () -> {
com.orbitz.consul.Consul consul = require(com.orbitz.consul.Consul.class);
});Do I make this change anyways ? |
|
Oh I see: |
|
Nice :) |
pierredavidbelanger
force-pushed
the
jooby-consul
branch
from
d6f55d4 to
086b2fb
Compare
|
Rebased on master and also squashed into a nice single commit |
2 participants
A very little something I plan to use in my application.
It is a module that expose a client to communicate with Consul, using the nice OrbitzWorldwide/consul-client lib.
I made the extra mile to make it generic (but not too much) enough to be reusable.
It is pretty straight forward (zero config) to use as a simple service registration with health check and service discovery module (this is what I intend to use it for).
You can see the doc I started here.
I will let you have a look at it.
If you like it, I will clean it up a little, add some tests and give it to you.
Do not hesitate to make me change everything, I am not afraid to refactor :)