Re: [jclouds] Support for OpenStack OS-Services API (#573)

2015-03-24 Thread Everett Toews 
Thanks for the pull request but it's release week in jclouds and that means 
it's time to clean up the PR queue. This PR will be over 6 months old as of 
April 1. If you intend to continue work on it, please make a comment by April 
2. Otherwise it will be closed on April 3.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-85716682

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-11-18 Thread inbar stolberg 
ping @jdaggett , @everett-toews 

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-63495357

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-28 Thread Avi Ben-Harush 
Pending further review

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60752076

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-23 Thread BuildHive 
[jclouds » jclouds 
#1832](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1832/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60217793

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-23 Thread CloudBees pull request builder plugin 
[jclouds-pull-requests 
#1320](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1320/) SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60211997

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-23 Thread Avi Ben-Harush 
Changes applied

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60209527

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-23 Thread CloudBees pull request builder plugin 
[jclouds-pull-requests-java-6 
#231](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/231/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60209531

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-23 Thread Avi Ben-Harush 
Thanks for the review @jdaggett.
unfortunatly not at the moment (lack of time). 
I'm working now on Ceilometer API, hopefully once it is done i can go back to 
this.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60203051

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
Thanks for the contribution @avibh ! Overall, it looks pretty good so far. :+1: 

Looking at docs/sources, this 
[extension](http://developer.openstack.org/api-ref-compute-v2-ext.html#ext-os-services)
 has two additional methods that are not in this PR:
- `PUT /v2/​{tenant_id}​/os-services/disable-log-reason`
- `GET /v2/​{tenant_id}​/os-services/detail`

Do you plan to add these as well?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60132546

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
> +   FluentIterable list();
> +
> +   /**
> +* enable an os-service
> +* @param host - host to enable
> +* @param binary - binary to enable
> +* @return os-service that was enabled
> +*/
> +   @Named("os-services:list")
> +   @PUT
> +   @Path("/enable")
> +   @SelectJson("service")
> +   @Produces(MediaType.APPLICATION_JSON)
> +   @Consumes(MediaType.APPLICATION_JSON)
> +   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> +   @MapBinder(BindToJsonPayload.class)

A shortcut for this would be to replace the MapBinder with: 
`@Payload("%7B\"host\":\"{host}\", \"binary\":\"{binary}\"%7D")` :+1: 

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19232979

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
> +
> +  public Builder disabledReason(String disabledReason) {
> + this.disabledReason = disabledReason;
> + return this;
> +  }
> +
> +  public Builder id(String id) {
> + this.id = id;
> + return this;
> +  }
> +
> +  public Service build() {
> + return new Service(binary, host, state, status, disabledReason, 
> updated, zone, id);
> +  }
> +
> +  public Builder fromOsService(Service in) {

Simplify this to `fromService` for consistency.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19232331

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
> +   }
> +
> +   public Builder toBuilder() {
> +  return new Builder().fromOsService(this);
> +   }
> +
> +   public static class Builder {
> +
> +  protected String binary;
> +  protected String host;
> +  protected State state;
> +  protected Status status;
> +  protected Date updated;
> +  protected String zone;
> +  protected String disabledReason;
> +  protected String id;

`id` is generally the first parameter in the OpenStack domain classes and APIs. 
Could you please reorder all instances of `id` to be the first parameter across 
the API? 

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19232323

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
> +   }
> +
> +   public Optional getUpdated() {
> +  return updated;
> +   }
> +
> +   public String getZone() {
> +  return zone;
> +   }
> +
> +   public String getId() {
> +  return id;
> +   }
> +
> +   @Override
> +   public boolean equals(Object o) {

In general, prefer `Objects.equals(...)`. Here is an example in the Nova 
[Network](https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/domain/Network.java#L75)
 class.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19230588

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
> +   @SelectJson("services")
> +   @Fallback(Fallbacks.EmptyFluentIterableOnNotFoundOr404.class)
> +   FluentIterable list();
> +
> +   /**
> +* enable an os-service
> +* @param host - host to enable
> +* @param binary - binary to enable
> +* @return os-service that was enabled
> +*/
> +   @Named("os-services:list")
> +   @PUT
> +   @Path("/enable")
> +   @SelectJson("service")
> +   @Produces(MediaType.APPLICATION_JSON)
> +   @Consumes(MediaType.APPLICATION_JSON)

The `@Consumes` annotation only needs to be applied to the top level interface, 
which you have already done on line 47. Please remove the other `@Consumes` 
annotations on the API methods.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19229490

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
> +@Extension(of = ServiceType.COMPUTE, namespace = 
> ExtensionNamespaces.SERVICES)
> +@RequestFilters(AuthenticateRequest.class)
> +@Consumes(MediaType.APPLICATION_JSON)
> +@Path("/os-services")
> +public interface ServicesApi {
> +
> +   /**
> +* List all os-services (binary, host, zone...)
> +*
> +* @return all os-services (binary, host, zone...)
> +*/
> +   @Named("os-services:list")
> +   @GET
> +   @SelectJson("services")
> +   @Fallback(Fallbacks.EmptyFluentIterableOnNotFoundOr404.class)
> +   FluentIterable list();

Upper wildcard unnecessary. `FluentIterable list();`

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19229118

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
> +  Service service = (Service) o;
> +
> +  if (binary != null ? !binary.equals(service.binary) : service.binary 
> != null) return false;
> +  if (host != null ? !host.equals(service.host) : service.host != null) 
> return false;
> +  if (state != null ? !state.equals(service.state) : service.state != 
> null) return false;
> +  if (status != null ? !status.equals(service.status) : service.status 
> != null) return false;
> +  if (disabledReason != null ? 
> !disabledReason.equals(service.disabledReason) : service.disabledReason != 
> null) return false;
> +  if (updated != null ? !updated.equals(service.updated) : 
> service.updated != null) return false;
> +  if (zone != null ? !zone.equals(service.zone) : service.zone != null) 
> return false;
> +  if (id != null ? !id.equals(service.id) : service.id != null) return 
> false;
> +
> +  return true;
> +   }
> +
> +   @Override
> +   public int hashCode() {

In general, prefer `Objects.hashCode(...)`. Here is an example in the Nova 
[Network](https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/domain/Network.java#L71)
 class.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19228903

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
> +   .binary(in.getBinary())
> +   .host(in.getHost())
> +   .state(in.getState())
> +   .status(in.getStatus())
> +   .disabledReason(in.getDisabledReason())
> +   .updated(in.getUpdated().isPresent() ? in.getUpdated().get() 
> : null)
> +   .zone(in.getZone())
> +   .id(in.getId());
> +  }
> +   }
> +
> +   protected String binary;
> +   protected String host;
> +   protected State state;
> +   protected Status status;
> +   protected String disabledReason;

I would annotate this with `@Named("disabled_reason")`

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19228027

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Jeremy Daggett 
> @@ -261,6 +262,15 @@
> Optional getConsolesApi(
>   @EndpointParam(parser = RegionToEndpoint.class) String region);
>  
> +
> +   /**
> +* Provides access to OS-Services features.
> +*/
> +   @Delegate
> +   Optional getServicesApi(

The upper wildcard is no longer necessary: `Optional`

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19227510

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread BuildHive 
[jclouds » jclouds 
#1826](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1826/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60058276

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread CloudBees pull request builder plugin 
[jclouds-pull-requests 
#1314](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1314/) SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60054870

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread CloudBees pull request builder plugin 
[jclouds-pull-requests-java-6 
#225](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/225/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60052238

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-22 Thread Avi Ben-Harush 
done changes as requested.


---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-60052125

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-20 Thread inbar stolberg 
> +import javax.ws.rs.PUT;
> +import javax.ws.rs.Path;
> +import javax.ws.rs.Produces;
> +import javax.ws.rs.core.MediaType;
> +
> +/**
> + * Provides access to OpenStack Compute (Nova) Services extension API.
> + */
> +@Beta
> +@Extension(of = ServiceType.COMPUTE, namespace = 
> ExtensionNamespaces.SERVICES)
> +@RequestFilters(AuthenticateRequest.class)
> +@Consumes(MediaType.APPLICATION_JSON)
> +@Path("/os-services")
> +public interface ServicesApi {
> +
> +

minor: remove empty line

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19079877

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-20 Thread inbar stolberg 
> +return valueOf(checkNotNull(state, "state").toUpperCase());
> + } catch (IllegalArgumentException e) {
> +return UNRECOGNIZED;
> + }
> +  }
> +   }
> +
> +   public static Builder builder() {
> +  return new ConcreteBuilder();
> +   }
> +
> +   public Builder toBuilder() {
> +  return new ConcreteBuilder().fromOsService(this);
> +   }
> +
> +   public abstract static class Builder> {

don't use abstract builder (old technical debt),
for examples see:
https://github.com/jclouds/jclouds/pull/560
https://github.com/jclouds/jclouds/pull/562 

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19079701

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-20 Thread inbar stolberg 
> +  protected ConcreteBuilder self() {
> + return this;
> +  }
> +   }
> +
> +   protected String binary;
> +   protected String host;
> +   protected State state;
> +   protected Status status;
> +   protected String disabledReason;
> +   @Named("updated_at")
> +   private final Optional updated;
> +   protected String zone;
> +   protected String id;
> +
> +   @ConstructorProperties({

minor change to:
@ConstructorProperties({"binary", "host", "state", "status", "disabled_reason", 
"updated_at", "zone", "id"})

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r19079651

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-19 Thread BuildHive 
[jclouds » jclouds 
#1809](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1809/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-59648251

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-19 Thread CloudBees pull request builder plugin 
[jclouds-pull-requests 
#1301](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1301/) SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-59647607

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-19 Thread CloudBees pull request builder plugin 
[jclouds-pull-requests-java-6 
#212](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/212/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-59646956

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-13 Thread Jeremy Daggett 
> +   }
> +
> +   public Builder toBuilder() {
> +  return new ConcreteBuilder().fromOsService(this);
> +   }
> +
> +   public abstract static class Builder> {
> +
> +  protected abstract T self();
> +
> +  protected String binary;
> +  protected String host;
> +  protected State state;
> +  protected Status status;
> +  protected Date updated;
> +  protected String zone;

According to the 
[sources](https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/contrib/services.py#L40),
 there is a `disabled_reason` that should also be present. Also, if the 
`os-extended-services-delete` extension is available, then there is also an 
`id` field exposed. We should probably take that into account.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573/files#r18779897

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-13 Thread Jeremy Daggett 
Thanks for the contribution! @avibh 

Nova Extension APIs have the "os" prefix stripped from them making this the 
`ServiceApi`. Can you please update the references to reflect that? Thx!

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-58918902

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-13 Thread BuildHive 
[jclouds » jclouds 
#1791](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1791/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-58854110

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-12 Thread CloudBees pull request builder plugin 
[jclouds-pull-requests 
#1288](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1288/) SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-58852067

Re: [jclouds] Support for OpenStack OS-Services API (#573)

2014-10-12 Thread CloudBees pull request builder plugin 
[jclouds-pull-requests-java-6 
#199](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/199/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/573#issuecomment-58850088