Initial push #2
Initial push #2
Conversation
* Intitial commit * Update gitignore * Implement review feedback * Add travis.yml * Add gradle wrappers * Renamed consulresolver -> consulnameresolver, added deploy script * move ossh credentials
| } | ||
|
|
||
| dependencies { | ||
| compile "io.grpc:grpc-all:${grpcVersion}" |
There was a problem hiding this comment.
From what I read at a glance here, you need either grpc-all or the other deps?
| testCompile "junit:junit:4.12" | ||
| testCompile 'org.hamcrest:hamcrest-library:1.3' | ||
| testCompile 'org.awaitility:awaitility:2.0.0' | ||
| testCompile 'com.pszymczyk.consul:embedded-consul:0.2.3' |
| private ImmutableCatalogOptions buildCatalogOptions() { | ||
| ImmutableCatalogOptions.Builder options = ImmutableCatalogOptions.builder(); | ||
|
|
||
| if (this.datacenter.isPresent()) { |
There was a problem hiding this comment.
For conciseness I'd use ifPresent here like so
this.datacenter.ifPresent(d -> options.datacenter(com.google.common.base.Optional.of(d)));
but that's just matter of taste though.
| options.datacenter(com.google.common.base.Optional.of(this.datacenter.get())); | ||
| } | ||
|
|
||
| if (this.tags.isPresent()) { |
There was a problem hiding this comment.
this.tags.ifPresent(tags -> tags.forEach(options::tag));
| return options.build(); | ||
| } | ||
|
|
||
| private void updateListenerWithEmptyResult() { |
There was a problem hiding this comment.
Probably rename method to something like propagateServiceUnavailableToListeners?
|
|
||
| public Builder(String service) { | ||
| if (service == null) { | ||
| throw new RuntimeException("Service cannot be null"); |
There was a problem hiding this comment.
Is a RuntimeException required here? Otherwise I favor a NullPointerException and go with
this.service = Preconditions.checkNotNull(service, "Service cannot be null");
| Optional<String> address = Optional.ofNullable(this.consulAddress); | ||
| Optional<String> datacenter = Optional.ofNullable(this.datacenter); | ||
| Optional<List<String>> tags; | ||
| if (this.tags.isEmpty()) { |
There was a problem hiding this comment.
Does using Optional<List<String>> have a special semantic here? If not, I'd rather go with just using an empty list.
This also applies to ConsulNameResolver.
| read -p "Enter the version (e.g. 0.0.1) " -r VERSION | ||
| if [ -z "$VERSION" ]; then | ||
| echo "Version cannot be empty!" | ||
| exit |
| @Rule | ||
| public ExpectedException thrown = ExpectedException.none(); | ||
|
|
||
| @Test |
There was a problem hiding this comment.
I've seen other test classes below omit the test prefix, probably the test methods in this class should be renamed to avoid the superfluous prefix.
| private final int port; | ||
|
|
||
| public PingPongServer() { | ||
| this(55551); |
There was a problem hiding this comment.
Probably better use a random value here to decrease possibility of already taken ports?
| public final Optional<List<String>> tags; | ||
|
|
||
| private ConsulQueryParameter(Optional<String> consulAddress, String service, | ||
| Optional<String> datacenter, Optional<List<String>> tags) { |
There was a problem hiding this comment.
private ConsulQueryParameter(Builder builder) {
| @Override | ||
| public void start(Listener listener) { | ||
| Preconditions.checkState(this.listener == null, "already started"); | ||
| this.listener = Preconditions.checkNotNull(listener, "listener"); |
There was a problem hiding this comment.
error message "listener"? Should probably be more informative: "listener cannot be empty".
| } | ||
|
|
||
| @Test | ||
| public void testSanitizeInputBadScheme() throws Exception { |
There was a problem hiding this comment.
please remove test prefix, like in other test class.
| echo "Deploying artifact to maven central" | ||
| read -p "Are you sure you want to deploy? (yes/NO)? " -r DO_DEPLOYMENT | ||
|
|
||
| if ( [ "$DO_DEPLOYMENT" == "yes" ] ); then |
No description provided.