You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Ilya Kulakov <no...@github.com> on 2013/11/13 08:58:07 UTC
[jclouds-labs] Cloudsigma v2 API Pagination (#35)
I'll squash the commits after you consider code is ready to be merged.
You can merge this Pull Request by running:
git pull https://github.com/cloudsigma/jclouds-labs cloudsigma2-api-pagination
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/35
-- Commit Summary --
* Add initial Cloudsigma v2 pagination support
* Remove server availability groups listing
* Replace Meta with PaginationOptions
* Fix expect test resources
* Add expect tests for methods returning PaginatedCollection
* Fix PaginationOptions license
-- File Changes --
M cloudsigma2/src/main/java/org/jclouds/cloudsigma2/CloudSigma2Api.java (259)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/domain/PaginatedCollection.java (60)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseDiscounts.java (74)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseDriveInfos.java (75)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseDrives.java (74)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseFirewallPolicies.java (95)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseIPInfos.java (74)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseIPs.java (74)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseLibraryDrives.java (74)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseLicenses.java (74)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseServerInfos.java (75)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseServers.java (75)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseSubscriptions.java (95)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseTags.java (95)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseTransactions.java (74)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/functions/internal/ParseVLANs.java (95)
A cloudsigma2/src/main/java/org/jclouds/cloudsigma2/options/PaginationOptions.java (122)
M cloudsigma2/src/test/java/org/jclouds/cloudsigma2/CloudSigma2ApiExpectTest.java (401)
M cloudsigma2/src/test/java/org/jclouds/cloudsigma2/CloudSigma2ApiLiveTest.java (43)
M cloudsigma2/src/test/resources/ledger.json (4)
M cloudsigma2/src/test/resources/libdrives.json (4)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/35.patch
https://github.com/jclouds/jclouds-labs/pull/35.diff
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
Commited to [master](https://git-wip-us.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=0bebb584662d30fd7f9b772f787feeebdc9d9d0a).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28476764
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public static class ToPagedIterable extends ArgsToPagedIterable<Discount, ToPagedIterable> {
> +
> + private CloudSigma2Api api;
> +
> + @Inject
> + public ToPagedIterable(CloudSigma2Api api) {
> + this.api = api;
> + }
> +
> + @Override
> + protected Function<Object, IterableWithMarker<Discount>> markerToNextForArgs(List<Object> args) {
> + return new Function<Object, IterableWithMarker<Discount>>() {
> + @Override
> + public IterableWithMarker<Discount> apply(@Nullable Object input) {
This function will only be called when the marker is present, so in practice the `input` parameter won't be null. Remove the `@Nullable` annotation (apply this to all parser classes).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35/files#r7619717
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ilya Kulakov <no...@github.com>.
@nacx Original "style" fix commit contained non-style fixes. That would be a lot of manual mangling of various chunks of code…
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28404276
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #89](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/89/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28401561
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #90](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/90/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28401572
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for clarifying @shevchenator. This one is good to merge then (will be good to have the fixes for these cases, in a different PR! :)).
Thanks for all the work!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28476376
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
A few fixes seem to have escaped the squash:
* The `@Nullable` annotations in the parsers are back again.
* The `PaginatedCollection` has the `@Inject` annotation.
* The `PaginationOptions` is again mutable and does not define the constant.
Could you kindly bring those fixes back and re-squash? That would be ideal, but if it is easier for you, I'm ok to leave the formatting thing out of this PR and open a new one.
Thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28401556
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #92](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/92/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28411960
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #88](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/88/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28393882
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
Changes look good and I think it is good to merge, if no one else has something to add.
Could you squash the commits in a way that the PR ends up with 2 commits: one with all the code style thing (as it is a general style refactor in the entire provider), and one that just adds the pagination? This way we can differentiate the commit that actually fixes the issue and properly reference it.
BTW, great job here, thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28395572
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ilya Kulakov <no...@github.com>.
@nacx I don't see the `@Inject` annotation in https://github.com/cloudsigma/jclouds-labs/blob/77beb27d9b5813ed022aa4a3350c3d83cced29dd/cloudsigma2/src/main/java/org/jclouds/cloudsigma2/options/PaginationOptions.java
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28402545
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.base.Optional;
> +import com.google.inject.Inject;
> +import org.jclouds.cloudsigma2.options.PaginationOptions;
> +import org.jclouds.collect.IterableWithMarker;
> +
> +import java.util.Iterator;
> +
> +/**
> + * @author Vladimir Shevchenko
> + */
> +public class PaginatedCollection<T> extends IterableWithMarker<T> {
> +
> + private Iterable<T> objects;
> + private PaginationOptions paginationOptions;
> +
> + @Inject
This constructor is manually called from the parsers and there is no argument here that is injected by Guice; remove the `@Inject` annotation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35/files#r7619747
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
@Kentzo You are right, all fixes are there (I didn't look at the entire changes).
The problem is that the points that I mentioned are contained in the "style" commit instead of being part of the commit that adds the pagination. There are two changes in this PR which should be self-contained: adding pagination, and applying the style to the provider. First one should contain the pagination and the fixes suggested regarding its implementation. The latter, should contain only the style changes.
Sounds good to you?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28403367
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ilya Kulakov <no...@github.com>.
@nacx Take a look.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28411590
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #580](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/580/) 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/pull/35#issuecomment-28401978
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
@Kentzo @shevchenator commits look good now. Thanks for the effort! I really appreciate that.
I've run the live tests on LVS, and there are three failures:
Failed tests:
testEditProfileInfo(org.jclouds.cloudsigma2.CloudSigma2LasVegasLiveTest): [{"error_point": "country", "error_type": "validation", "error_message": "Value 'USA' is not a valid choice."}]
testEditVLAN(org.jclouds.cloudsigma2.CloudSigma2LasVegasLiveTest): position (0) must be less than the number of elements that remained (0)
testEditIP(org.jclouds.cloudsigma2.CloudSigma2LasVegasLiveTest): position (0) must be less than the number of elements that remained (0)
First one does not seem o be related with this change (so I'm ok to leave it out of this PR), but the others seem to be related to pagination, or to how the assertions are performed. Could you confirm that and see if there is something that should be fixed there?
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28475212
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ilya Kulakov <no...@github.com>.
@nacx I think we've addressed issues you mentioned.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28394565
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
> + * limitations under the License.
> + */
> +package org.jclouds.cloudsigma2.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +import javax.inject.Named;
> +import java.beans.ConstructorProperties;
> +
> +/**
> + * @author Vladimir Shevchenko
> + */
> +public class PaginationOptions extends BaseHttpRequestOptions {
> +
> + public static class Builder{
> + private int limit = 20;
Move to a `DEFAULT_LIMIT` constant?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35/files#r7619801
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #581](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/581/) 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/pull/35#issuecomment-28403127
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #93](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/93/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28419646
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.json.Json;
> +
> +import javax.inject.Inject;
> +import javax.inject.Singleton;
> +import java.beans.ConstructorProperties;
> +import java.util.List;
> +
> +/**
> + * @author Vladimir Shevchenko
> + */
> +@Singleton
> +public class ParseVLANs extends ParseJson<ParseVLANs.VLANs> {
> + static class VLANs extends PaginatedCollection<VLANInfo> {
> +
> + @ConstructorProperties({"objects", "meta"})
> + VLANs(Iterable<VLANInfo> objects, PaginationOptions paginationOptions) {
Why is this constructor package protected?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35/files#r7619782
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> + }
> +
> + @Test
> + public void testListLicensesPaginatedCollection() throws Exception {
> + CloudSigma2Api api = requestSendsResponse(
> + getBuilder()
> + .endpoint(endpoint + "licenses/?limit=3&offset=3")
> + .build()
> + , responseBuilder()
> + .payload(payloadFromResourceWithContentType("/licences.json", MediaType.APPLICATION_JSON))
> + .build());
> +
> + for(License license : api.listLicenses(new PaginationOptions.Builder().limit(3).offset(3).build())){
> + assertNotNull(license);
> + }
> }
> }
You can test that the pagination stuff works with the expect tests. Take a look at [this example in the Abiquo provider](https://github.com/jclouds/jclouds-labs/blob/master/abiquo/src/test/java/org/jclouds/abiquo/features/CloudApiExpectTest.java#L64-L101), to see how you can do it.
Basically, to test the "listAll" method (the one that returns the `PagedIterable`), you configure two requests: the initial one, and the "auto-generated" request for the next page (each returning a single page). This way you can call the "listAll" methods, iterate the list, and verify that it contains the elements in both pages (the advancing logic and the underlying request are working).
Could you add these tests so the unit tests properly verify the page advancing mechanism is working properly?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35/files#r7620008
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #586](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/586/) 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/pull/35#issuecomment-28420261
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ilya Kulakov <no...@github.com>.
@nacx And PaginationOptions seems to be immutable: https://github.com/cloudsigma/jclouds-labs/blob/77beb27d9b5813ed022aa4a3350c3d83cced29dd/cloudsigma2/src/main/java/org/jclouds/cloudsigma2/options/PaginationOptions.java
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28403174
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #585](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/585/) 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/pull/35#issuecomment-28412701
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Andrew Phillips <no...@github.com>.
> Commited to master.
Thanks for all the reviewing time, @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28810371
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public Builder offset(int offset){
> + this.offset = offset;
> + return this;
> + }
> +
> + public PaginationOptions build(){
> + return new PaginationOptions(limit, offset, 0);
> + }
> + }
> +
> + private int limit;
> + private int offset;
> + @Named("total_count")
> + private final int totalCount;
Can we mark this field as final, and remove the "setters" in this class to make it immutable (and move the setter logic to the `build()` method)? The `PaginatedCollection` creates a new instance of the `PaginationOptions` when advancing, so there is no need to make it mutable.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35/files#r7619851
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Vladimir Shevchenko <no...@github.com>.
Sure, @nacx ! I'll add fixes to another PR.
Thanks for your help & consideration!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28476489
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Vladimir Shevchenko <no...@github.com>.
Hey @nacx according to first failure seems like Cloudsigma added a data validation for this field, I'll fix it later.
About those 2 left, seems like your IP & VLAN subscription is expired. I can't buy ones from via API methods. IP & VLAN purchases are available only in web interface. I'll add handler for this too.
Anyway all of those 3 failures are not related to pagination.
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28475818
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
Great job here @Kentzo @shevchenator, this looks pretty good! It is not trivial to understand how the whole pagination thing works :) In order to merge this, it is important to add the expect tests I mentioned. To summarize the next steps:
- [ ] Address minor review comments.
- [ ] Add the "page advance" tests to the expect tests.
- [ ] Format the code to follow our style guide: 3 space indentation and 120 characters line length (and also add a space before the `{`.
Many thanks for the contribution!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28377835
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ilya Kulakov <no...@github.com>.
@nacx what about now?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28419299
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
All fixes are in place except the Expect tests for the "page advance" (they can be found in the previous [style commit](https://github.com/cloudsigma/jclouds-labs/blob/5927d10dc30aa871934f3db9f60d512931df5d5f/cloudsigma2/src/test/java/org/jclouds/cloudsigma2/CloudSigma2ApiExpectTest.java)). I don't want to bother you, really, just want to make sure everything is properly put together.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28413446
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
I completely understand, but it is also important that commits are self-contained. Imagine a case where we had to revert, say, the style commit. We would leave the pagination feature without the fixes, and no one could predict that (it is not a realistic example just no note that not having self-contained commits has its implications).
I understand that manual changes are required to have the two clean commits, and that might not be a nice work to do, but it is not a crazy amount of work. It is very focused at the parser classes, the expect test class, and the PaginationCollection and PaginationOptions classes.
In the end what matters is that the commit history of the project is consistent.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28406315
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ilya Kulakov <no...@github.com>.
@nacx I also don't see the `@Nullable` annotation in `functions/internal`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28402933
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by Ignasi Barrera <no...@github.com>.
@shevchenator You are right, I've purchased some IP and VLANs and now the only test failing is the `testEditProfileInfo` one, so the others are working as expected. Great!
It would be great to handle the subscription case and throw a `SkipException` to mark those tests as skipped if the user has no IPs or VLANs available.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28477184
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #578](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/578/) 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/pull/35#issuecomment-28394419
Re: [jclouds-labs] Cloudsigma v2 API Pagination (#35)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #87](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/87/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/35#issuecomment-28374511