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 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-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 
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 
#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 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-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-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 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 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 Jeremy Daggett 
 @@ -261,6 +262,15 @@
 OptionalConsolesApi getConsolesApi(
   @EndpointParam(parser = RegionToEndpoint.class) String region);
  
 +
 +   /**
 +* Provides access to OS-Services features.
 +*/
 +   @Delegate
 +   Optional? extends ServicesApi getServicesApi(

The upper wildcard is no longer necessary: `OptionalServicesApi`

---
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 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 
 +  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 
 +@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? extends Service list();

Upper wildcard unnecessary. `FluentIterableService 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 
 +   @SelectJson(services)
 +   @Fallback(Fallbacks.EmptyFluentIterableOnNotFoundOr404.class)
 +   FluentIterable? extends Service 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 
 +   }
 +
 +   public OptionalDate 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 
 +   }
 +
 +   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 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 
 +   FluentIterable? extends Service 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 
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-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 OptionalDate 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-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 BuilderT extends BuilderT {

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 
 +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-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-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 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

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

2014-10-13 Thread Avi Ben-Harush 

You can merge this Pull Request by running:

  git pull https://github.com/avibh/jclouds-1 apache-master

Or you can view, comment on it, or merge it online at:

  https://github.com/jclouds/jclouds/pull/573

-- Commit Summary --

  * Support for OpenStack OS-Services API

-- File Changes --

M 
apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/NovaApi.java 
(10)
M 
apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/config/NovaHttpApiModule.java
 (2)
A 
apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/domain/OsService.java
 (235)
M 
apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/extensions/ExtensionNamespaces.java
 (5)
A 
apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/extensions/OsServicesApi.java
 (93)
A 
apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/extensions/OsServicesApiExpectTest.java
 (75)
A 
apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/extensions/OsServicesApiLiveTest.java
 (87)
M apis/openstack-nova/src/test/resources/extension_list_full.json (8)
A apis/openstack-nova/src/test/resources/os_services_list.json (36)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/573.patch
https://github.com/jclouds/jclouds/pull/573.diff

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

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

2014-10-13 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

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

2014-10-13 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-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-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 Jeremy Daggett 
 +   }
 +
 +   public Builder? toBuilder() {
 +  return new ConcreteBuilder().fromOsService(this);
 +   }
 +
 +   public abstract static class BuilderT extends BuilderT {
 +
 +  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