jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial Network API implementation (#389)
Date Fri, 23 Jun 2017 06:53:02 GMT
nacx requested changes on this pull request.

Just some minor final comments, but it looks quite good. Thanks, @trevorflanagan!

> +      this.state = state;
+   }
+
+   @Override
+   public boolean apply(String networkDomainId) {
+      checkNotNull(networkDomainId, "networkDomainId");
+      logger.trace("looking for state on network domain %s", networkDomainId);
+      final NetworkDomain networkDomain = networkApi.getNetworkDomain(networkDomainId);
+
+      if (networkDomain == null && state == State.DELETED) {
+         return true;
+      }
+
+      if (networkDomain.state().isFailed()) {
+         throw new IllegalStateException(
+               MessageFormat.format("Network Domain {0} is in FAILED state", networkDomain.id()));

This looks like more a check for the caller? What if the expected state passed to the constructor
is a "failed" one? I mean, although it is unlikely we want individual classes to have meaningful
and complete method signatures.

> +      this.networkApi = networkApi;
+      this.state = state;
+   }
+
+   @Override
+   public boolean apply(String vlanId) {
+      checkNotNull(vlanId, "vlanId");
+      logger.trace("looking for state on vlan %s", vlanId);
+      final Vlan vlan = networkApi.getVlan(vlanId);
+
+      if (vlan == null && state == State.DELETED) {
+         return true;
+      }
+
+      if (vlan.state().isFailed()) {
+         throw new IllegalStateException(MessageFormat.format("Vlan {0} is in FAILED state",
vlan.id()));

Same comment as above.

> +import org.jclouds.json.Json;
+import org.jclouds.logging.Logger;
+
+import javax.annotation.Resource;
+import java.io.InputStream;
+
+import static org.jclouds.http.HttpUtils.releasePayload;
+
+public class ParseResponse implements Function<HttpResponse, String> {
+
+   @Resource
+   protected Logger logger = Logger.NULL;
+   protected final Json json;
+   protected String propertyName;
+
+   public ParseResponse(Json json, String propertyName) {

Make this protected?

> +import org.jclouds.http.HttpResponse;
+import org.jclouds.http.HttpResponseException;
+import org.jclouds.json.Json;
+import org.jclouds.logging.Logger;
+
+import javax.annotation.Resource;
+import java.io.InputStream;
+
+import static org.jclouds.http.HttpUtils.releasePayload;
+
+public class ParseResponse implements Function<HttpResponse, String> {
+
+   @Resource
+   protected Logger logger = Logger.NULL;
+   protected final Json json;
+   protected String propertyName;

Make it final too.

> +   String tryFindInfoPropertyValue(Response response) {
+      if (!response.info().isEmpty()) {
+         Optional<String> optionalPropertyName = FluentIterable.from(response.info())
+               .firstMatch(new Predicate<Property>() {
+                  @Override
+                  public boolean apply(Property input) {
+                     return input.name().equals(propertyName);
+                  }
+               }).transform(new Function<Property, String>() {
+                  @Override
+                  public String apply(Property input) {
+                     return input.value();
+                  }
+               });
+         if (!optionalPropertyName.isPresent()) {
+            throw new IllegalStateException();

Add a message here so the HttpResponseException you propagate above has all the info.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/389#pullrequestreview-45907911
Mime
View raw message