Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
Closed #166. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#event-272752086
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
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-labs-openstack/pull/166#issuecomment-85718143
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+ protected Gson gson; + + @Inject + public BindQueryRequestPayload() { + gson = new GsonBuilder().disableHtmlEscaping().create(); + } + + @Override + public R extends HttpRequest R bindToRequest(R request, MapString, Object postParams) { + MapString, Object map = Maps.newHashMap(); + + ComplexQueryOptions options = (ComplexQueryOptions)postParams.get(options); + + if (options != null) { + + if (!isNullOrEmptyString(options.getFilter())) { Fortunately, there is a utility method `Strings.isNullOrEmpty(value)` that can be used here instead. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r20578452
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+ + if (options.getLimit() != null) { +map.put(limit, options.getLimit()); + } + + if (!isNullOrEmptyString(options.getOrderBy())) { +map.put(orderby, options.getOrderBy()); + } + } + + request.setPayload(gson.toJson(map)); + request.getPayload().getContentMetadata().setContentType(application/json); + return request; + } + + private boolean isNullOrEmptyString(Object value) { You can remove this method as per previous comment about the utility method --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r20578491
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+ + if (!isNullOrEmptyString(options.getFilter())) { +map.put(filter, options.getFilter()); + } + + if (options.getLimit() != null) { +map.put(limit, options.getLimit()); + } + + if (!isNullOrEmptyString(options.getOrderBy())) { +map.put(orderby, options.getOrderBy()); + } + } + + request.setPayload(gson.toJson(map)); + request.getPayload().getContentMetadata().setContentType(application/json); Prefer: `request.getPayload().getContentMetadata().setContentType(MediaType.APPLICATION_JSON)` --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r20578458
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+import javax.inject.Named; +import javax.ws.rs.Consumes; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.Produces; +import javax.ws.rs.core.MediaType; +import java.util.List; + + +/** + * Provides access to Meters features. + * + */ +@RequestFilters(AuthenticateRequest.class) +@Consumes(MediaType.APPLICATION_JSON) +@Path(/v2/meters) `@Path(/meters)` --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r20578787
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+ */ +@RequestFilters(AuthenticateRequest.class) +@Consumes(MediaType.APPLICATION_JSON) +@Path(/v2/meters) +public interface MeterApi { + + @Named(resource:listMeters) + @GET + @Fallback(EmptyListOnNotFoundOr404.class) + ListMeter list(); + + @Named(resource:listMeters) + @GET + @Produces(MediaType.APPLICATION_JSON) + @Fallback(EmptyListOnNotFoundOr404.class) + ListMeter list(QueryOptions... options); We have been moving away from varargs, however this API may need them. Leave this for now and we can revisit. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r20578722
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+import javax.ws.rs.Path; +import javax.ws.rs.Produces; +import javax.ws.rs.core.MediaType; +import java.util.List; + + +/** + * Provides access to Meters features. + * + */ +@RequestFilters(AuthenticateRequest.class) +@Consumes(MediaType.APPLICATION_JSON) +@Path(/v2/query/) +public interface QueryApi { + + @Path(samples) `@Path(/samples)` Please change line 57 as well --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r20578866
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+import javax.inject.Named; +import javax.ws.rs.Consumes; +import javax.ws.rs.POST; +import javax.ws.rs.Path; +import javax.ws.rs.Produces; +import javax.ws.rs.core.MediaType; +import java.util.List; + + +/** + * Provides access to Meters features. + * + */ +@RequestFilters(AuthenticateRequest.class) +@Consumes(MediaType.APPLICATION_JSON) +@Path(/v2/query/) `@Path(/query)` --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r20578834
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+package org.jclouds.openstack.ceilometer.v2; + +import org.jclouds.View; +import org.jclouds.apis.internal.BaseApiMetadataTest; +import org.testng.annotations.Test; + +import com.google.common.collect.ImmutableSet; +import com.google.common.reflect.TypeToken; + +/** + * Tests the Ceilometer {@link ApiMetadata}. + */ +@Test(groups = unit, testName = CeilometerApiMetadataTest) +public class CeilometerApiMetadataTest extends BaseApiMetadataTest { + public CeilometerApiMetadataTest() { + super(new org.jclouds.openstack.ceilometer.v2.CeilometerApiMetadata(), ImmutableSet.TypeToken? extends View of()); No need for fully qualified class name here --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r20579327
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
@avibh Thanks for your patience, this is definitely progressing! As I previously mentioned, we are moving to use AutoValue for value objects to simplify development. Would you be up to making those changes in a follow on PR? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-63655407
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
ping @jdaggett , @everett-toews ? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-63495674
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
@jdaggett Good to go here? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-63423589
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
fixed merge confilicts --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-63218871
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds-labs-openstack-pull-requests #495](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/495/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-63219011
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds » jclouds-labs-openstack #1976](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1976/) 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-labs-openstack/pull/166#issuecomment-63219241
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds-labs-openstack-pull-requests #476](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/476/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-61772901
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds » jclouds-labs-openstack #1946](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1946/) 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-labs-openstack/pull/166#issuecomment-61773594
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
changed the MetersApi.listMeters to MetersApi.list but left the QueryOptions as is cause of a new ComplexQueryOptions for QueryApi . changed QueryApi so it uses ComplexQueryOptions and used MapBinder to controle the post params. waiting for further comments --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-61773903
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
Hi @jdaggett I've managed to overcome most of the issues using the following signature MeterApi.listMeters(QueryOptions... options) where QueryOptions is extending the BaseHttpRequestOptions class. i'm now left with the following - meters and samples can get query params, problem is they cant be sent as null/empty string since ceilometer fails. example - {filter: , orderby: , limit: } need a way to avoid sending empty query params, any idea? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-61399081
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
OK @avibh! Let's help answer the 3 questions you posed earlier. The Ceilometer API started following some practices that we see in the Neutron APIs today. Specifically filters, queries, and sorting of results. We need to address this by augment `PaginationOptions` and potentially add some option classes. 1) Based on what I have read about the [`Simple`](http://docs.openstack.org/developer/ceilometer/webapi/v2.html#simple-query) versus [`Complex`](http://docs.openstack.org/developer/ceilometer/webapi/v2.html#complex-query) queries, I believe that you can use [`PaginationOptions.queryParameters`](https://github.com/jclouds/jclouds/blob/master/apis/openstack-keystone/src/main/java/org/jclouds/openstack/v2_0/options/PaginationOptions.java#L37) to achieve the necessary results. At first pass, that is where I would start. 2) For the `MetersApi`, I would change the signatures to align with the other OpenStack APIs. As I mentioned in a previous comment, I would name it `MeterApi` and provide both a `list()` and a `list(ListOptions)`. It is simple and concise. :+1: 3) You should be able to utilize the `*Option` related classes in jclouds for binding the query params to the request. There are all sorts of these option classes in the codebase that you can use for referencethat allow you to pass/bind query parameters in the method. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-61118728
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
Thanks for the review @jdaggett , I've applied the changes (except for the cool @AutoValue feature). any idea on the open issues I've wrote on the PR? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-60751524
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds-labs-openstack-pull-requests #471](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/471/) UNSTABLE Looks like there's a problem with this pull request --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-60751862
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds » jclouds-labs-openstack #1922](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1922/) 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-labs-openstack/pull/166#issuecomment-60752852
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds-labs-openstack-pull-requests #472](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/472/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-60788674
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds » jclouds-labs-openstack #1923](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1923/) 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-labs-openstack/pull/166#issuecomment-60789945
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds-labs-openstack-pull-requests #473](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/473/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-60793542
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
Thanks for the contribution @avibh! I am going to start reviewing this PR this afternoon. I am not sure if you are aware of some changes being made to greatly simplify development of new APIs/providers in jclouds with a library called AutoValue. Since we are in a bit of flux there, I would really like to see it utilized in the Ceilometer API. Let's get through the first review, and then I can share more on how to best approach moving this forward! --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-60673531
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+import java.util.Set; + +import org.jclouds.location.Region; +import org.jclouds.location.functions.RegionToEndpoint; +import org.jclouds.openstack.ceilometer.v2.features.MetersApi; +import org.jclouds.openstack.ceilometer.v2.features.QueryApi; +import org.jclouds.rest.annotations.Delegate; +import org.jclouds.rest.annotations.EndpointParam; + +import com.google.inject.Provides; + +/** + * Provides access to the OpenStack Orchestration (Heat) API. + * + */ +public interface CeilometerApi extends Closeable { Please annotate the API interfaces in this PR with `@Beta` --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r19452579
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+descriptionjclouds components to access an implementation of OpenStack Ceilometer/description +packagingbundle/packaging + +properties +!-- keystone endpoint -- + test.openstack-ceilometer.endpointhttp://localhost:5000/v2.0//test.openstack-ceilometer.endpoint + test.openstack-ceilometer.api-version1/test.openstack-ceilometer.api-version +test.openstack-ceilometer.build-version / + test.openstack-ceilometer.identityFIXME_IDENTITY/test.openstack-ceilometer.identity + test.openstack-ceilometer.credentialFIXME_CREDENTIALS/test.openstack-ceilometer.credential + test.jclouds.keystone.credential-typepasswordCredentials/test.jclouds.keystone.credential-type + jclouds.osgi.exportorg.jclouds.openstack.ceilometer.v2*;version=${project.version}/jclouds.osgi.export + jclouds.osgi.importorg.jclouds*;version=${project.version},*/jclouds.osgi.import +/properties + +repositories The `repositories` section can be removed since it is no longer necessary. I need to pull it out of the Heat pom too :+1: --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r19452571
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
+ .credentialName(${password}) + .documentation(URI.create(https://wiki.openstack.org/wiki/Ceilometer;)) + .version(2) + .endpointName(Keystone base url ending in /v2.0/) + .defaultEndpoint(http://localhost:5000/v2.0/;) + .defaultProperties(org.jclouds.openstack.ceilometer.v2.CeilometerApiMetadata.defaultProperties()) + .defaultModules(ImmutableSet.Class? extends Modulebuilder() + .add(AuthenticationApiModule.class) + .add(KeystoneAuthenticationModule.class) + .add(RegionModule.class) + .add(CeilometerHttpApiModule.class).build()); + } + + @Override + public org.jclouds.openstack.ceilometer.v2.CeilometerApiMetadata build() { + return new org.jclouds.openstack.ceilometer.v2.CeilometerApiMetadata(this); The full package names are not necessary. `CeilometerApiMetadata` should suffice on the previous 2 lines. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166/files#r19452617
[jclouds-labs-openstack] OS Ceilometer API support (#166)
OpenStack Ceilometer API#39;s support (Skeleton amp; meters/samples API) open issues I need help with - 1. meters and samples can get query params, problem is they cant be sent as null/empty string since ceilometer fails. example - {quot;filterquot;: quot;quot;, quot;orderbyquot;: quot;quot;, quot;limitquot;: quot;quot;} need a way to avoid sending empty query params 2. for meters API - currently I#39;ve created quot;MetersApi.listMeters(QueryOptions... options)quot; should I follow this notion or simplify it as quot;MetersApi.listMeters(String filter)quot; where filter is a json filter as in item #1 You can merge this Pull Request by running: git pull https://github.com/avibh/jclouds-labs-openstack ceilometer-support Or you can view, comment on it, or merge it online at: https://github.com/jclouds/jclouds-labs-openstack/pull/166 -- Commit Summary -- * OS Ceilometer API support -- File Changes -- M README.md (1) A openstack-ceilometer/README.md (15) A openstack-ceilometer/pom.xml (145) A openstack-ceilometer/src/main/java/org/jclouds/openstack/ceilometer/v2/CeilometerApi.java (53) A openstack-ceilometer/src/main/java/org/jclouds/openstack/ceilometer/v2/CeilometerApiMetadata.java (90) A openstack-ceilometer/src/main/java/org/jclouds/openstack/ceilometer/v2/config/CeilometerHttpApiModule.java (44) A openstack-ceilometer/src/main/java/org/jclouds/openstack/ceilometer/v2/domain/Meter.java (199) A openstack-ceilometer/src/main/java/org/jclouds/openstack/ceilometer/v2/domain/Sample.java (259) A openstack-ceilometer/src/main/java/org/jclouds/openstack/ceilometer/v2/features/MetersApi.java (59) A openstack-ceilometer/src/main/java/org/jclouds/openstack/ceilometer/v2/features/QueryApi.java (62) A openstack-ceilometer/src/main/java/org/jclouds/openstack/ceilometer/v2/handlers/CeilometerErrorHandler.java (52) A openstack-ceilometer/src/main/java/org/jclouds/openstack/ceilometer/v2/options/QueryOptions.java (102) A openstack-ceilometer/src/main/resources/META-INF/services/org.jclouds.apis.ApiMetadata (18) A openstack-ceilometer/src/test/java/org/jclouds/openstack/ceilometer/v2/CeilometerApiMetadataTest.java (34) A openstack-ceilometer/src/test/java/org/jclouds/openstack/ceilometer/v2/features/MetersApiLiveTest.java (72) A openstack-ceilometer/src/test/java/org/jclouds/openstack/ceilometer/v2/features/MetersApiMockTest.java (116) A openstack-ceilometer/src/test/java/org/jclouds/openstack/ceilometer/v2/features/SamplesApiLiveTest.java (74) A openstack-ceilometer/src/test/java/org/jclouds/openstack/ceilometer/v2/features/SamplesApiMockTest.java (116) A openstack-ceilometer/src/test/java/org/jclouds/openstack/ceilometer/v2/internal/BaseCeilometerApiLiveTest.java (44) A openstack-ceilometer/src/test/java/org/jclouds/openstack/ceilometer/v2/internal/BaseCeilometerApiMockTest.java (39) A openstack-ceilometer/src/test/resources/access.json (242) A openstack-ceilometer/src/test/resources/logback.xml (56) A openstack-ceilometer/src/test/resources/meters_list_query_response.json (6502) A openstack-ceilometer/src/test/resources/meters_list_response.json (6502) A openstack-ceilometer/src/test/resources/samples_list_response.json (337) M pom.xml (1) -- Patch Links -- https://github.com/jclouds/jclouds-labs-openstack/pull/166.patch https://github.com/jclouds/jclouds-labs-openstack/pull/166.diff --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds-labs-openstack-pull-requests #469](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/469/) UNSTABLE Looks like there's a problem with this pull request --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-60514494
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds » jclouds-labs-openstack #1908](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1908/) 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-labs-openstack/pull/166#issuecomment-60514655
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds-labs-openstack-pull-requests #470](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/470/) FAILURE Looks like there's a problem with this pull request --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs-openstack/pull/166#issuecomment-60514833
Re: [jclouds-labs-openstack] OS Ceilometer API support (#166)
[jclouds » jclouds-labs-openstack #1909](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1909/) FAILURE 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-labs-openstack/pull/166#issuecomment-60515003