You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Zack Shoylev <no...@github.com> on 2014/05/08 01:27:53 UTC
[jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Note that one of the tests does not pass!
You can merge this Pull Request by running:
git pull https://github.com/rackerlabs/jclouds-labs-openstack broken-fallback-test
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-openstack/pull/96
-- Commit Summary --
* Adds paging fail tests for Queues.
-- File Changes --
M openstack-marconi/src/test/java/org/jclouds/openstack/marconi/v1/features/QueueApiMockTest.java (37)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-openstack/pull/96.patch
https://github.com/jclouds/jclouds-labs-openstack/pull/96.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> EmptyPaginatedCollectionOnNotFoundOr404 returns PaginatedCollection<Object>
> However Queues extends PaginatedCollection<Queue> and not Object.
Without having taking a long look at the code...should EPCONFO404 not be a generic class, i.e. `EmptyPaginatedCollectionOnNotFoundOr404<T> returns PaginatedCollection<T>`? I guess we can't change the declaration to ` EmptyPaginatedCollectionOnNotFoundOr404 returns PaginatedCollection<? extends Object>` - or, if we could, that probably wouldn't fix it.
As far as an "evil quick fix" idea goes: what if you cast the result to `(PaginatedCollection)` (i.e. kill the generic)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-42521627
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #283](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/283/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45377319
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1252](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1252/) 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/96#issuecomment-45386016
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #284](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/284/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45386131
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> + * contributor license agreements. See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +package org.jclouds.openstack.marconi.v1.fallbacks;
[minor] Remove blank line?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13473630
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
About to merge.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45386440
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
merged. does this need to be backported? I am thinking yes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45386868
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #279](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/279/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45292211
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1104](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1104/) 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/96#issuecomment-42699762
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> @@ -178,6 +179,42 @@ public void listOnePageOfQueues() throws Exception {
> }
> }
>
> + public void listOnePageOfQueuesFail() throws Exception {
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(new MockResponse().setBody(accessRackspace));
> + server.enqueue(new MockResponse().setResponseCode(404).setBody(""));
> +
> + try {
> + MarconiApi api = api(server.getUrl("/").toString(), "openstack-marconi");
> + QueueApi queueApi = api.getQueueApiForZoneAndClient("DFW", CLIENT_ID);
> +
> + List<Queue> queues = queueApi.list(false).concat().toList();
Just curious...as compared to the following test, why the `toList()` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r12417715
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> + * contributor license agreements. See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +package org.jclouds.openstack.marconi.v1.fallbacks;
> Add newline between package and import.
Yes, sure. Sorry if I wasn't clear there - I was referring to the blank line between the license header and the `package` statement.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13522303
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1243](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1243/) 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/96#issuecomment-45292241
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.rest.ResourceNotFoundException;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import static com.google.common.base.Throwables.propagate;
> +import static com.google.common.util.concurrent.Futures.immediateFuture;
> +import static org.jclouds.http.HttpUtils.contains404;
> +import static org.jclouds.util.Throwables2.getFirstThrowableOfType;
> +
> +public class EmptyQueuesFallback implements Fallback<Queues> {
> +
> + public ListenableFuture<Queues> create(Throwable t) throws Exception {
> + return immediateFuture(createOrPropagate(t));
> + }
> +
> + public Queues createOrPropagate(Throwable t) throws Exception {
> +
[minor] remove blank line?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13473647
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> @@ -34,10 +35,7 @@
> import java.util.UUID;
>
> import static org.jclouds.openstack.marconi.v1.options.ListQueuesOptions.Builder.limit;
> -import static org.testng.Assert.assertEquals;
> -import static org.testng.Assert.assertFalse;
> -import static org.testng.Assert.assertNotNull;
> -import static org.testng.Assert.assertTrue;
> +import static org.testng.Assert.*;
Do we do wildcard imports? I thought not?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13473721
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
Thanks for the update, @zack-shoylev!
The PR builder reports 6 [Checkstyle violations](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/279/violations/), by the way, but none seem related to this PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45295191
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds-labs-openstack #1092 UNSTABLE
I'm guessing this is the [expected test failure](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/org.apache.jclouds.labs$openstack-marconi/1092/testReport/junit/org.jclouds.openstack.marconi.v1.features/QueueApiMockTest/listPagedIterableCollectionQueuesFail/), @zack-shoylev?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-42520596
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> + server.shutdown();
> + }
> + }
> +
> + public void listPagedIterableCollectionQueuesFail() throws Exception {
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(new MockResponse().setBody(accessRackspace));
> + server.enqueue(new MockResponse().setResponseCode(404).setBody(""));
> +
> + try {
> + MarconiApi api = api(server.getUrl("/").toString(), "openstack-marconi");
> + QueueApi queueApi = api.getQueueApiForZoneAndClient("DFW", CLIENT_ID);
> +
> + Queues queues = queueApi.list(ListQueuesOptions.NONE);
> +
> + assertEquals(queues.size(), 0);
`assertTrue(queues.isEmpty(), "Expecting empty queues but was %s", queues)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r12417719
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #223](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/223/) 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/96#issuecomment-42499651
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
> +import static org.jclouds.util.Throwables2.getFirstThrowableOfType;
> +
> +public class EmptyQueuesFallback implements Fallback<Queues> {
> +
> + public ListenableFuture<Queues> create(Throwable t) throws Exception {
> + return immediateFuture(createOrPropagate(t));
> + }
> +
> + public Queues createOrPropagate(Throwable t) throws Exception {
> +
> + return valOnNotFoundOr404(Queues.EMPTY,
> + checkNotNull(t, "throwable"));
> + }
> +
> + public static Queues valOnNotFoundOr404(Queues val, Throwable t) {
> + if (containsResourceNotFoundException(checkNotNull(t, "throwable")) || contains404(t)) {
The two helper methods are based on very similar methods in Fallbacks, which is why they are here like that.
https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/Fallbacks.java#L190
I guess it makes sense to refactor them here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13494813
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #285](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/285/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45386772
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
Good eye. Applying fixes and then rebase/merge. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45384230
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Everett Toews <no...@github.com>.
Please backport.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45387056
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1253](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1253/) 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/96#issuecomment-45386769
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
I don't have much time to look at this this week. However, I believe using a QueuesFallback would be the way to fix this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-43689804
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> @@ -114,7 +114,7 @@
> @GET
> @ResponseParser(ParseQueues.class)
> @Consumes(MediaType.APPLICATION_JSON)
> - @Fallback(EmptyPaginatedCollectionOnNotFoundOr404.class)
> + @Fallback(NullOnNotFoundOr404.class)
> @Path("queues")
> Queues list(ListQueuesOptions options);
Should we mark this as `@Nullable Queues`, then, or at least add a comment to that effect in the Javadoc?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r12496920
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Jeremy Daggett <no...@github.com>.
Once you have addressed the feedback from @everett-toews +1 !
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45384314
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #281](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/281/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45377131
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Everett Toews <no...@github.com>.
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import static com.google.common.base.Throwables.propagate;
> +import static com.google.common.util.concurrent.Futures.immediateFuture;
> +import static org.jclouds.http.HttpUtils.contains404;
> +import static org.jclouds.util.Throwables2.getFirstThrowableOfType;
> +
> +public class EmptyQueuesFallback implements Fallback<Queues> {
> +
> + public ListenableFuture<Queues> create(Throwable t) throws Exception {
> + return immediateFuture(createOrPropagate(t));
> + }
> +
> + @Override
> + public Queues createOrPropagate(Throwable t) throws Exception {
> + if ( ( getFirstThrowableOfType(checkNotNull(t, "throwable"), ResourceNotFoundException.class) != null ) || contains404(t)) {
Remove unnecessary spaces.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13508780
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
I tried switching the EmptyPaginatedCollectionOnNotFoundOr404 to generic but had issues using the class in the annotation then. The wildcard ? didn't seem to work as a fix. I haven't tried the evil fix yet, but would prefer to avoid it (for now).
I will have another look at switching the fallback to a generic today.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-42564226
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> + server.shutdown();
> + }
> + }
> +
> + public void listPagedIterableCollectionQueuesFail() throws Exception {
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(new MockResponse().setBody(accessRackspace));
> + server.enqueue(new MockResponse().setResponseCode(404).setBody(""));
> +
> + try {
> + MarconiApi api = api(server.getUrl("/").toString(), "openstack-marconi");
> + QueueApi queueApi = api.getQueueApiForZoneAndClient("DFW", CLIENT_ID);
> +
> + Queues queues = queueApi.list(ListQueuesOptions.NONE);
> +
> + assertTrue(queues.isEmpty(), "Queues should have been empty");
Use same message as above?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13473738
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Everett Toews <no...@github.com>.
> + * contributor license agreements. See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +package org.jclouds.openstack.marconi.v1.fallbacks;
Add newline between package and import.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13508697
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Everett Toews <no...@github.com>.
> @@ -178,6 +180,43 @@ public void listOnePageOfQueues() throws Exception {
> }
> }
>
> + public void listOnePageOfQueuesFail() throws Exception {
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(new MockResponse().setBody(accessRackspace));
> + server.enqueue(new MockResponse().setResponseCode(404).setBody(""));
setBody() is unnecessary.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13508648
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import static com.google.common.base.Throwables.propagate;
> +import static com.google.common.util.concurrent.Futures.immediateFuture;
> +import static org.jclouds.http.HttpUtils.contains404;
> +import static org.jclouds.util.Throwables2.getFirstThrowableOfType;
> +
> +public class EmptyQueuesFallback implements Fallback<Queues> {
> +
> + public ListenableFuture<Queues> create(Throwable t) throws Exception {
> + return immediateFuture(createOrPropagate(t));
> + }
> +
> + public Queues createOrPropagate(Throwable t) throws Exception {
> +
> + return valOnNotFoundOr404(Queues.EMPTY,
> + checkNotNull(t, "throwable"));
[minor] Does this fit on one line?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13473639
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #224](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/224/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-42700010
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Ignasi Barrera <no...@github.com>.
Yep. You can't have parameterized classes in the annotations, as the type of the generic parameter belongs to the instance of the object and not the definition of the class, which is what you need in the annotations.
I don't like the idea of returning `null`, as the api will behave different than any other existing APIs. Why not just creating a custom `QueuesFallback` for this method that implements `Fallback<Queues>`? The code would be pretty much the same than the one in the `EmptyPaginatedCollectionOnNotFoundOr404 ` class, but that isn't a big deal IMO and the API will behave as expected. That fallback in keystone can be refactored in another PR to make it non-final and more generic so it can be easily subclassed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-42783051
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
I don't know how to fix this (without introducing a lot of code).
The problem is that the fallback for Queues list(ListQueuesOptions options); is EmptyPaginatedCollectionOnNotFoundOr404
EmptyPaginatedCollectionOnNotFoundOr404 returns PaginatedCollection<Object>
However Queues extends PaginatedCollection<Queue> and not Object.
What is the clean way to fix this? Note: this problem probably occurs in other places that use EmptyPaginatedCollectionOnNotFoundOr404
Another note: I would not mind if the API just returned a null instead of an empty collection.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-42496159
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> +import static org.jclouds.util.Throwables2.getFirstThrowableOfType;
> +
> +public class EmptyQueuesFallback implements Fallback<Queues> {
> +
> + public ListenableFuture<Queues> create(Throwable t) throws Exception {
> + return immediateFuture(createOrPropagate(t));
> + }
> +
> + public Queues createOrPropagate(Throwable t) throws Exception {
> +
> + return valOnNotFoundOr404(Queues.EMPTY,
> + checkNotNull(t, "throwable"));
> + }
> +
> + public static Queues valOnNotFoundOr404(Queues val, Throwable t) {
> + if (containsResourceNotFoundException(checkNotNull(t, "throwable")) || contains404(t)) {
Is this called from places other than `createOrPropagate` above?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13473664
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1249](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1249/) 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/96#issuecomment-45378421
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Andrew Phillips <no...@github.com>.
> @@ -178,6 +179,42 @@ public void listOnePageOfQueues() throws Exception {
> }
> }
>
> + public void listOnePageOfQueuesFail() throws Exception {
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(new MockResponse().setBody(accessRackspace));
> + server.enqueue(new MockResponse().setResponseCode(404).setBody(""));
> +
> + try {
> + MarconiApi api = api(server.getUrl("/").toString(), "openstack-marconi");
> + QueueApi queueApi = api.getQueueApiForZoneAndClient("DFW", CLIENT_ID);
> +
> + List<Queue> queues = queueApi.list(false).concat().toList();
> +
> + assertEquals(queues.size(), 0);
`assertTrue(queues.isEmpty(), "Expecting empty queues but was %s", queues)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r12417696
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1241](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1241/) 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/96#issuecomment-45278263
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1092](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1092/) 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/96#issuecomment-42496504
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
> @@ -178,6 +179,42 @@ public void listOnePageOfQueues() throws Exception {
> }
> }
>
> + public void listOnePageOfQueuesFail() throws Exception {
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(new MockResponse().setBody(accessRackspace));
> + server.enqueue(new MockResponse().setResponseCode(404).setBody(""));
> +
> + try {
> + MarconiApi api = api(server.getUrl("/").toString(), "openstack-marconi");
> + QueueApi queueApi = api.getQueueApiForZoneAndClient("DFW", CLIENT_ID);
> +
> + List<Queue> queues = queueApi.list(false).concat().toList();
I suspect I copied this from one of the other tests and didn't fully refactor. Can be fixed :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r12434842
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
I will have time to look more later this week.
Note: this is not the only place this fallback is used. Which is why I would prefer to not do that QueuesFallback approach.
How would you refactor the generic keystone fallback? You can't parametrize it as a generic, so I am not sure what the right approach is.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-42851528
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
> import org.jclouds.rest.binders.BindToJsonPayload;
>
> import javax.inject.Named;
> -import javax.ws.rs.Consumes;
> -import javax.ws.rs.DELETE;
> -import javax.ws.rs.GET;
> -import javax.ws.rs.PUT;
> -import javax.ws.rs.Path;
> -import javax.ws.rs.PathParam;
> -import javax.ws.rs.Produces;
> -import javax.ws.rs.QueryParam;
> +import javax.ws.rs.*;
Will fix.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13506839
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Everett Toews <no...@github.com>.
> + QueueApi queueApi = api.getQueueApiForZoneAndClient("DFW", CLIENT_ID);
> +
> + //List<Queue> queues = queueApi.list(false).concat().toList();
> + FluentIterable<Queue> queues = queueApi.list(false).concat();
> +
> + assertTrue(queues.isEmpty(), "Expecting empty queues but was " + queues.toString());
> + }
> + finally {
> + server.shutdown();
> + }
> + }
> +
> + public void listPagedIterableCollectionQueuesFail() throws Exception {
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(new MockResponse().setBody(accessRackspace));
> + server.enqueue(new MockResponse().setResponseCode(404).setBody(""));
setBody() is unnecessary.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13508655
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #278](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/278/) 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/96#issuecomment-45277407
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
> + server.shutdown();
> + }
> + }
> +
> + public void listPagedIterableCollectionQueuesFail() throws Exception {
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(new MockResponse().setBody(accessRackspace));
> + server.enqueue(new MockResponse().setResponseCode(404).setBody(""));
> +
> + try {
> + MarconiApi api = api(server.getUrl("/").toString(), "openstack-marconi");
> + QueueApi queueApi = api.getQueueApiForZoneAndClient("DFW", CLIENT_ID);
> +
> + Queues queues = queueApi.list(ListQueuesOptions.NONE);
> +
> + assertTrue(queues.isEmpty(), "Queues should have been empty");
Good catch
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13494678
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Zack Shoylev <no...@github.com>.
Okay this is another iteration of this problem taking review concerns into account.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45291895
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Everett Toews <no...@github.com>.
+1
Once all of the (relatively minor) review comments have been addresses.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45380865
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by Everett Toews <no...@github.com>.
> @@ -178,6 +180,43 @@ public void listOnePageOfQueues() throws Exception {
> }
> }
>
> + public void listOnePageOfQueuesFail() throws Exception {
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(new MockResponse().setBody(accessRackspace));
> + server.enqueue(new MockResponse().setResponseCode(404).setBody(""));
> +
> + try {
> + MarconiApi api = api(server.getUrl("/").toString(), "openstack-marconi");
> + QueueApi queueApi = api.getQueueApiForZoneAndClient("DFW", CLIENT_ID);
> +
> + //List<Queue> queues = queueApi.list(false).concat().toList();
Delete commented line of code.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96/files#r13508652
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1247](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1247/) 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/96#issuecomment-45376888
Re: [jclouds-labs-openstack] Adds paging fail tests for Queues. (#96)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #282](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/282/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/96#issuecomment-45377194