You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by snotling <no...@github.com> on 2015/01/14 23:35:44 UTC
[jclouds-labs-google] Make deleteObject() return false on 404
response (#122)
You can merge this Pull Request by running:
git pull https://github.com/snotling/jclouds-labs-google patch-2
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-google/pull/122
-- Commit Summary --
* Make deleteObject() return false on 404 response
-- File Changes --
M google-cloud-storage/src/main/java/org/jclouds/googlecloudstorage/features/ObjectApi.java (5)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-google/pull/122.patch
https://github.com/jclouds/jclouds-labs-google/pull/122.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122
Re: [jclouds-labs-google] Make deleteObject() return false on 404
response (#122)
Posted by danbroudy <no...@github.com>.
Im adding ObjectApiMockTest because there doesn't appear to be a unit test and it would be good to convert GCS to Mock
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122#issuecomment-73935078
Re: [jclouds-labs-google] Make deleteObject() return false on 404
response (#122)
Posted by Andrew Gaul <no...@github.com>.
> @@ -199,7 +198,7 @@ GoogleCloudStorageObject simpleUpload(@PathParam("bucket") String bucketName, @H
> @Named("Object:delete")
> @DELETE
> @Path("storage/v1/b/{bucket}/o/{object}")
> - @Fallback(TrueOnNotFoundOr404.class)
> + @Fallback(FalseOnNotFoundOr404.class)
Which test exercises this functionality?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r22977919
Re: [jclouds-labs-google] Make deleteObject() and deleteBucket()
return false on 404 response (#122)
Posted by Andrew Gaul <no...@github.com>.
@snotling Jenkins only runs unit tests for us. Unfortunately integration tests ar a manual process due to various automation issues.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122#issuecomment-88474208
Re: [jclouds-labs-google] Make deleteObject() and deleteBucket()
return false on 404 response (#122)
Posted by Andrew Gaul <no...@github.com>.
> @@ -422,8 +425,9 @@ public void testDeleteObjectWithOptions() {
>
> @AfterClass
> private void deleteBucket() {
> - api.getBucketApi().deleteBucket(BUCKET_NAME);
> - api.getBucketApi().deleteBucket(BUCKET_NAME2);
> + assertTrue(api.getBucketApi().deleteBucket(BUCKET_NAME));
> + assertTrue(api.getBucketApi().deleteBucket(BUCKET_NAME2));
> + assertFalse(api.getBucketApi().deleteBucket(NONEXISTENT_BUCKET_NAME));
Should `@AfterClass` have asserts? This method should just clean up after the test mehod. Instead can you create a separate `testDeleteBucket` method?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r27321367
Re: [jclouds-labs-google] Make deleteObject() and deleteBucket()
return false on 404 response (#122)
Posted by Andrew Gaul <no...@github.com>.
Ran integration tests locally, squashed commits, and pushed to master as 6f9783ce4c316d93d6eac209967cc0a86152844f and 1.9.x as cbb6e88294696225da7e97f40e72aa26239c2651.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122#issuecomment-88487455
Re: [jclouds-labs-google] Make deleteObject() and deleteBucket()
return false on 404 response (#122)
Posted by Andrew Gaul <no...@github.com>.
Closed #122.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122#event-270641394
Re: [jclouds-labs-google] Make deleteObject() return false on 404
response (#122)
Posted by Andrew Gaul <no...@github.com>.
> @@ -199,7 +198,7 @@ GoogleCloudStorageObject simpleUpload(@PathParam("bucket") String bucketName, @H
> @Named("Object:delete")
> @DELETE
> @Path("storage/v1/b/{bucket}/o/{object}")
> - @Fallback(TrueOnNotFoundOr404.class)
> + @Fallback(FalseOnNotFoundOr404.class)
@snotling I opened [JCLOUDS-805](https://issues.apache.org/jira/browse/JCLOUDS-805) to track this issue. I will take a quick look and asked @hsbhathiya, the original developer, to do the same.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r23048033
Re: [jclouds-labs-google] Make deleteObject() and deleteBucket()
return false on 404 response (#122)
Posted by Bhathiya <no...@github.com>.
> @@ -422,8 +425,9 @@ public void testDeleteObjectWithOptions() {
>
> @AfterClass
> private void deleteBucket() {
> - api.getBucketApi().deleteBucket(BUCKET_NAME);
> - api.getBucketApi().deleteBucket(BUCKET_NAME2);
> + assertTrue(api.getBucketApi().deleteBucket(BUCKET_NAME));
> + assertTrue(api.getBucketApi().deleteBucket(BUCKET_NAME2));
> + assertFalse(api.getBucketApi().deleteBucket(NONEXISTENT_BUCKET_NAME));
I think just removing asserts would be enough. Delete bucket tests are available in BucketApiLiveTests
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r27332165
Re: [jclouds-labs-google] Make deleteObject() return false on 404
response (#122)
Posted by snotling <no...@github.com>.
> @@ -199,7 +198,7 @@ GoogleCloudStorageObject simpleUpload(@PathParam("bucket") String bucketName, @H
> @Named("Object:delete")
> @DELETE
> @Path("storage/v1/b/{bucket}/o/{object}")
> - @Fallback(TrueOnNotFoundOr404.class)
> + @Fallback(FalseOnNotFoundOr404.class)
I've extended the "return-false-on-404" behavior to `deleteBucket()` as well.
And added a couple of unit tests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r26200794
Re: [jclouds-labs-google] Make deleteObject() return false on 404
response (#122)
Posted by Andrew Gaul <no...@github.com>.
> @@ -199,7 +198,7 @@ GoogleCloudStorageObject simpleUpload(@PathParam("bucket") String bucketName, @H
> @Named("Object:delete")
> @DELETE
> @Path("storage/v1/b/{bucket}/o/{object}")
> - @Fallback(TrueOnNotFoundOr404.class)
> + @Fallback(FalseOnNotFoundOr404.class)
@snotling The GCS tests now pass without error; can you rebase your commit and revisit this pull request with new tests?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r26175133
Re: [jclouds-labs-google] Make deleteObject() return false on 404
response (#122)
Posted by snotling <no...@github.com>.
> @@ -199,7 +198,7 @@ GoogleCloudStorageObject simpleUpload(@PathParam("bucket") String bucketName, @H
> @Named("Object:delete")
> @DELETE
> @Path("storage/v1/b/{bucket}/o/{object}")
> - @Fallback(TrueOnNotFoundOr404.class)
> + @Fallback(FalseOnNotFoundOr404.class)
In all the tests for GCS, the return value of ``deleteObject()`` calls is not checked.
So I think the tests should be improved in that regard.
Is that what you meant?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r22996584
Re: [jclouds-labs-google] Make deleteObject() return false on 404
response (#122)
Posted by snotling <no...@github.com>.
> @@ -199,7 +198,7 @@ GoogleCloudStorageObject simpleUpload(@PathParam("bucket") String bucketName, @H
> @Named("Object:delete")
> @DELETE
> @Path("storage/v1/b/{bucket}/o/{object}")
> - @Fallback(TrueOnNotFoundOr404.class)
> + @Fallback(FalseOnNotFoundOr404.class)
Is the current ``master`` GCS test suite supposed to pass? I'm having a few errors, in particular in ``ObjectApiLiveTest`` (the class I'm working on).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r23041040
Re: [jclouds-labs-google] Make deleteObject() and deleteBucket()
return false on 404 response (#122)
Posted by Damiano Albani <no...@github.com>.
All tests now pass.
By the way, am I overlooking something or Jenkins only shows the overall build status and not the actual errors when the integration tests fail?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122#issuecomment-88435017
Re: [jclouds-labs-google] Make deleteObject() and deleteBucket()
return false on 404 response (#122)
Posted by Damiano Albani <no...@github.com>.
> @@ -422,8 +425,9 @@ public void testDeleteObjectWithOptions() {
>
> @AfterClass
> private void deleteBucket() {
> - api.getBucketApi().deleteBucket(BUCKET_NAME);
> - api.getBucketApi().deleteBucket(BUCKET_NAME2);
> + assertTrue(api.getBucketApi().deleteBucket(BUCKET_NAME));
> + assertTrue(api.getBucketApi().deleteBucket(BUCKET_NAME2));
> + assertFalse(api.getBucketApi().deleteBucket(NONEXISTENT_BUCKET_NAME));
Indeed, having a dedicated test method is more appropriate.
I'll do that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r27322339
Re: [jclouds-labs-google] Make deleteObject() return false on 404
response (#122)
Posted by Andrew Gaul <no...@github.com>.
> @@ -199,7 +198,7 @@ GoogleCloudStorageObject simpleUpload(@PathParam("bucket") String bucketName, @H
> @Named("Object:delete")
> @DELETE
> @Path("storage/v1/b/{bucket}/o/{object}")
> - @Fallback(TrueOnNotFoundOr404.class)
> + @Fallback(FalseOnNotFoundOr404.class)
Definitely, can you either modify an existing test or add a new test? We need not check at all call sites, but at least the basic delete test should do this. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/122/files#r23021488