You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Jeremy Daggett <no...@github.com> on 2014/03/25 22:50:46 UTC
[jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN (#85)
This PR provides several updates the Rackspace Cloud Files CDN functionality.
- refactor of StaticWeb related headers - pulled up from CloudFilesHeaders to SwiftHeaders
- cleaned up tests and removed dead code
JIRA:
https://issues.apache.org/jira/browse/JCLOUDS-423
You can merge this Pull Request by running:
git pull https://github.com/rackerlabs/jclouds-labs-openstack cloudfiles-cdn
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-openstack/pull/85
-- Commit Summary --
* Moved Static Web related headers up to SwiftHeaders
* Adds CDN Directory Type support to UpdateCDNContainerOptions, removed builder(), and updated unit test.
* Update CDN unit and live tests
* Cleaned up dead code
-- File Changes --
M openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/reference/SwiftHeaders.java (9)
M rackspace-cloudfiles/src/main/java/org/jclouds/rackspace/cloudfiles/v1/options/UpdateCDNContainerOptions.java (86)
M rackspace-cloudfiles/src/main/java/org/jclouds/rackspace/cloudfiles/v1/reference/CloudFilesHeaders.java (9)
M rackspace-cloudfiles/src/test/java/org/jclouds/rackspace/cloudfiles/v1/features/CloudFilesCDNApiLiveTest.java (70)
M rackspace-cloudfiles/src/test/java/org/jclouds/rackspace/cloudfiles/v1/features/CloudFilesCDNApiMockTest.java (264)
A rackspace-cloudfiles/src/test/java/org/jclouds/rackspace/cloudfiles/v1/options/UpdateCDNContainerOptionsTest.java (111)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-openstack/pull/85.patch
https://github.com/jclouds/jclouds-labs-openstack/pull/85.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #201](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/201/) 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/85#issuecomment-39377329
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -86,6 +117,45 @@ public void testGet() throws Exception {
> }
> }
>
> + /**
> + * By default, this method is disabled due to daily purge limitations of 25 objects per day.
> + */
> + @Test(enabled = false)
> + public void testPurgeObject() throws Exception {
> + for (String regionId : regions) {
> + String objectName = "testPurge";
> + Payload payload = Payloads.newByteSourcePayload(ByteSource.wrap(new byte[]{1,2,3}));
Fixed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11129916
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> for (String regionId : regions) {
> - CDNApi cdnApi = api.cdnApiInRegion(regionId);
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
Ah, I was looking at the wrong API ;-) So `enable` returns a **Uri**, eh? Not exactly intuitive? Is this what the underlying API returns, too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11227541
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> - CDNContainer cdnContainer = api.cdnApiInRegion(regionId).get(name);
> - assertNotNull(cdnContainer);
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertCDNContainerNotNull(container);
> + assertTrue(container.isEnabled());
> + }
> + }
> +
> + public void testPurgeObject() throws Exception {
> + for (String regionId : regions) {
> + String objectName = "testPurge";
> + Payload payload = Payloads.newByteSourcePayload(ByteSource.wrap(new byte[] {1,2,3}));
> + ObjectApi objectApi = api.objectApiInRegionForContainer(regionId, name);
> +
> + // create a new object
> + assertNotNull(objectApi.replace(objectName, payload, ImmutableMap.<String, String>of()));
Assertion removed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11251234
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> @@ -43,6 +56,24 @@ public CloudFilesCDNApiLiveTest() {
> super();
> }
>
> + public void testEnable() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
> + }
> + }
> +
> + public void testEnableWithTTL() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name, 777777));
> + }
> + }
> +
> + public void testDisable() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).disable(name));
Any more specific assertions we can make in these three tests?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r10955870
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -86,6 +117,45 @@ public void testGet() throws Exception {
> }
> }
>
> + /**
> + * By default, this method is disabled due to daily purge limitations of 25 objects per day.
> + */
> + @Test(enabled = false)
Because you can only purge 25 objects per day, I felt that this test should be disabled. As long as you don't run the live tests more than 25 times a day, enabling this should be fine.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11129835
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> assertAuthentication(server);
> assertRequest(server.takeRequest(), "PUT", "/v1/MossoCloudFS_5bcf396e-39dd-45ff-93a1-712b9aba90a9/container-1");
> + } finally {
> + server.shutdown();
> + }
> + }
> +
> + public void testEnableFail() throws Exception {
Which failure are we testing here? Something to do with a 404? In that case, describe that in the test name?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r10956016
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> @@ -105,34 +123,64 @@ public void testListWithOptions() throws Exception {
> }
> }
>
> - public void testEnableAndDisable() throws Exception {
> + public void testListWithOptionsFail() throws Exception {
Similar comments as above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r10955997
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #187](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/187/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-38626630
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #210](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/210/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-39454977
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> }
> }
>
> + public void testUpdate() throws Exception {
> + for (String regionId : regions) {
> + // enable with a ttl
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name, 777777));
`assertTrue`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11156506
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Everett Toews <no...@github.com>.
Closed #85.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> for (String regionId : regions) {
> - CDNApi cdnApi = api.cdnApiInRegion(regionId);
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
No, because it returns a URI :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11224274
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> - CDNContainer cdnContainer = api.cdnApiInRegion(regionId).get(name);
> - assertNotNull(cdnContainer);
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertCDNContainerNotNull(container);
> + assertTrue(container.isEnabled());
> + }
> + }
> +
> + public void testPurgeObject() throws Exception {
> + for (String regionId : regions) {
> + String objectName = "testPurge";
> + Payload payload = Payloads.newByteSourcePayload(ByteSource.wrap(new byte[] {1,2,3}));
> + ObjectApi objectApi = api.objectApiInRegionForContainer(regionId, name);
> +
> + // create a new object
> + assertNotNull(objectApi.replace(objectName, payload, ImmutableMap.<String, String>of()));
replace() returns the etag of the object that was just created.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11224462
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Everett Toews <no...@github.com>.
+1
Merge away when other reviewers are happy.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-39466124
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #985](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/985/) 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/85#issuecomment-39143262
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> for (String regionId : regions) {
> - CDNApi cdnApi = api.cdnApiInRegion(regionId);
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
> +
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertCDNContainerNotNull(container);
Fine with an `assertNotNull` here - see explanation below. Sorry for the additional work!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11156442
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> for (String regionId : regions) {
> - CDNApi cdnApi = api.cdnApiInRegion(regionId);
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
> +
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertCDNContainerNotNull(container);
> + assertTrue(container.isEnabled());
> + }
> + }
> +
> + public void testEnableWithTTL() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name, 777777));
Should this be `assertTrue`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11156419
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> for (String regionId : regions) {
> - CDNApi cdnApi = api.cdnApiInRegion(regionId);
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
> +
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertCDNContainerNotNull(container);
No worries, that is why we have reviews.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11224317
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> @@ -81,6 +77,28 @@ public void testList() throws Exception {
> }
> }
>
> + public void testListFail() throws Exception {
What failure exactly are we testing here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r10955987
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -43,6 +56,24 @@ public CloudFilesCDNApiLiveTest() {
> super();
> }
>
> + public void testEnable() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
> + }
> + }
> +
> + public void testEnableWithTTL() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name, 777777));
> + }
> + }
> +
> + public void testDisable() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).disable(name));
Always!? ;) Added an assert method to the class that checks all of the CDN Container fields.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11129912
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> @@ -43,6 +56,24 @@ public CloudFilesCDNApiLiveTest() {
> super();
> }
>
> + public void testEnable() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
> + }
> + }
> +
> + public void testEnableWithTTL() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name, 777777));
> + }
> + }
> +
> + public void testDisable() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).disable(name));
Sorry, I didn't express myself clearly, and didn't check the test clearly enough :-( `disable` returns a boolean, from what I can see? So should we be testing for `true` here?
I don't think we need to check (here or below) that **all** fields are non-null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11156413
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #212](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/212/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-39506059
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #191](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/191/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-39143472
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1018](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1018/) 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/85#issuecomment-39506578
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1004](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1004/) 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/85#issuecomment-39377241
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #952](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/952/) 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/85#issuecomment-38626548
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -105,34 +123,64 @@ public void testListWithOptions() throws Exception {
> }
> }
>
> - public void testEnableAndDisable() throws Exception {
> + public void testListWithOptionsFail() throws Exception {
Updating test method to **testListWithOptionsIsEmpty()**
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11136852
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/access.json"))));
> + server.enqueue(addCommonHeaders(new MockResponse().setResponseCode(404).setBody(stringFromResource("/cdn_container_list.json"))));
> +
> + try {
> + CloudFilesApi api = api(server.getUrl("/").toString(), "rackspace-cloudfiles");
> + CDNApi cdnApi = api.cdnApiInRegion("DFW");
> +
> + FluentIterable<CDNContainer> cdnContainers = cdnApi.list();
> +
> + assertEquals(server.getRequestCount(), 2);
> + assertAuthentication(server);
> + assertRequest(server.takeRequest(), "GET", "/v1/MossoCloudFS_5bcf396e-39dd-45ff-93a1-712b9aba90a9/?format=json&enabled_only=true");
> +
> + assertNotNull(cdnContainers);
> + assertEquals(cdnContainers.size(), 0);
`assertTrue(cdnContainers.isEmpty)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r10955981
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
Thanks, @jdaggett! My comments were only minor - we can leave as-is or address as part of a separate PR later.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-39656725
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1015](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1015/) 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/85#issuecomment-39454982
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
Also getting a merge conflict warning...guess this needs a rebase?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-39196279
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> + MockWebServer server = mockOpenStackServer();
> + server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/access.json"))));
> + server.enqueue(addCommonHeaders(new MockResponse().setResponseCode(404).setBody(stringFromResource("/cdn_container_list.json"))));
> +
> + try {
> + CloudFilesApi api = api(server.getUrl("/").toString(), "rackspace-cloudfiles");
> + CDNApi cdnApi = api.cdnApiInRegion("DFW");
> +
> + FluentIterable<CDNContainer> cdnContainers = cdnApi.list();
> +
> + assertEquals(server.getRequestCount(), 2);
> + assertAuthentication(server);
> + assertRequest(server.takeRequest(), "GET", "/v1/MossoCloudFS_5bcf396e-39dd-45ff-93a1-712b9aba90a9/?format=json&enabled_only=true");
> +
> + assertNotNull(cdnContainers);
> + assertEquals(cdnContainers.size(), 0);
Fixed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11129922
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> - CDNContainer cdnContainer = api.cdnApiInRegion(regionId).get(name);
> - assertNotNull(cdnContainer);
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertCDNContainerNotNull(container);
> + assertTrue(container.isEnabled());
> + }
> + }
> +
> + public void testPurgeObject() throws Exception {
> + for (String regionId : regions) {
> + String objectName = "testPurge";
> + Payload payload = Payloads.newByteSourcePayload(ByteSource.wrap(new byte[] {1,2,3}));
> + ObjectApi objectApi = api.objectApiInRegionForContainer(regionId, name);
> +
> + // create a new object
> + assertNotNull(objectApi.replace(objectName, payload, ImmutableMap.<String, String>of()));
What is `replace` supposed to return? The old content? If we are trying to test `replace`, verify the expected return value here? Otherwise, if this is just part of the "test setup", remove the assertion?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11156492
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> - CDNContainer cdnContainer = api.cdnApiInRegion(regionId).get(name);
> - assertNotNull(cdnContainer);
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertCDNContainerNotNull(container);
> + assertTrue(container.isEnabled());
> + }
> + }
> +
> + public void testPurgeObject() throws Exception {
> + for (String regionId : regions) {
> + String objectName = "testPurge";
> + Payload payload = Payloads.newByteSourcePayload(ByteSource.wrap(new byte[] {1,2,3}));
> + ObjectApi objectApi = api.objectApiInRegionForContainer(regionId, name);
> +
> + // create a new object
> + assertNotNull(objectApi.replace(objectName, payload, ImmutableMap.<String, String>of()));
I guess we don't know what the etag is, but what does a non-null check buy us here? The "real" thing we are testing is the purge? In which case, just remove the assertion here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11227652
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -81,6 +77,28 @@ public void testList() throws Exception {
> }
> }
>
> + public void testListFail() throws Exception {
Updating test method to **testListIsEmpty()**
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11136851
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> return this;
> }
>
> public static class Builder {
> - /** @see UpdateCDNContainerOptions#ttl */
> + /**
> + * @see UpdateCDNContainerOptions#ttl
That comment about the @see tag was in some of the model classes. If you look through the code base, most of the Options classes have the @see tags. Is the thinking that we should remove these?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11127984
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Everett Toews <no...@github.com>.
Merged to master and 1.7.x
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-39509178
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Zack Shoylev <no...@github.com>.
> + }
> +
> + public void testUpdate() throws Exception {
> + for (String regionId : regions) {
> + UpdateCDNContainerOptions opts = new UpdateCDNContainerOptions();
> + opts.ttl(1234567);
> + opts.logRetention(true);
> + opts.enabled(false);
> +
> + // update the container
> + assertTrue(api.cdnApiInRegion(regionId).update(name, opts));
> +
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertEquals(container.getTtl(), 1234567);
> + assertTrue(container.isLogRetentionEnabled());
> + assertFalse(container.isEnabled());
I would consider changing the test instead. Is that feasible?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11004022
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> for (String regionId : regions) {
> - CDNApi cdnApi = api.cdnApiInRegion(regionId);
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
Should this be `assertTrue`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11156427
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> for (String regionId : regions) {
> - CDNApi cdnApi = api.cdnApiInRegion(regionId);
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name));
> +
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertCDNContainerNotNull(container);
> + assertTrue(container.isEnabled());
> + }
> + }
> +
> + public void testEnableWithTTL() throws Exception {
> + for (String regionId : regions) {
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name, 777777));
Same as above. Returns a URI...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11224307
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> assertAuthentication(server);
> assertRequest(server.takeRequest(), "PUT", "/v1/MossoCloudFS_5bcf396e-39dd-45ff-93a1-712b9aba90a9/container-1");
> + } finally {
> + server.shutdown();
> + }
> + }
> +
> + public void testEnableFail() throws Exception {
I followed the format of the newer test naming like this [one](https://github.com/jclouds/jclouds-labs-openstack/blob/master/rackspace-autoscale/src/test/java/org/jclouds/rackspace/autoscale/v1/features/WebhookApiMockTest.java#L73)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11224619
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
In general, thanks a lot for the cleanup, @jdaggett! Just some questions about test naming: there are a lot of `...Fail()` tests that do not obviously seem to fail (e.g. they don't expect an exception). Could we give them more specific names to describe which kind of failure on the server they are intended to handle?
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-38626726
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> @@ -86,6 +117,45 @@ public void testGet() throws Exception {
> }
> }
>
> + /**
> + * By default, this method is disabled due to daily purge limitations of 25 objects per day.
> + */
> + @Test(enabled = false)
> + public void testPurgeObject() throws Exception {
> + for (String regionId : regions) {
> + String objectName = "testPurge";
> + Payload payload = Payloads.newByteSourcePayload(ByteSource.wrap(new byte[]{1,2,3}));
[minor] Space before `{`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r10955879
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #1005](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/1005/) 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/85#issuecomment-39379094
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Zack Shoylev <no...@github.com>.
> return this;
> }
>
> public static class Builder {
> - /** @see UpdateCDNContainerOptions#ttl */
> + /**
> + * @see UpdateCDNContainerOptions#ttl
There was a discussion regarding @see vs text in another PR recently. What is the right approach?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11004177
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> }
> }
>
> + public void testUpdate() throws Exception {
> + for (String regionId : regions) {
> + // enable with a ttl
> + assertNotNull(api.cdnApiInRegion(regionId).enable(name, 777777));
Same... returns a URI
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11224480
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #202](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/202/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85#issuecomment-39379085
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> @@ -86,6 +117,45 @@ public void testGet() throws Exception {
> }
> }
>
> + /**
> + * By default, this method is disabled due to daily purge limitations of 25 objects per day.
> + */
> + @Test(enabled = false)
Do we want to commit tests that are disabled? What is the intention behind these? Do we think many users will be able to use this somehow?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r10955955
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Jeremy Daggett <no...@github.com>.
> + }
> +
> + public void testUpdate() throws Exception {
> + for (String regionId : regions) {
> + UpdateCDNContainerOptions opts = new UpdateCDNContainerOptions();
> + opts.ttl(1234567);
> + opts.logRetention(true);
> + opts.enabled(false);
> +
> + // update the container
> + assertTrue(api.cdnApiInRegion(regionId).update(name, opts));
> +
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertEquals(container.getTtl(), 1234567);
> + assertTrue(container.isLogRetentionEnabled());
> + assertFalse(container.isEnabled());
I rewrote the test!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r11136735
Re: [jclouds-labs-openstack] Updates to Rackspace Cloud Files CDN
(#85)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + public void testUpdate() throws Exception {
> + for (String regionId : regions) {
> + UpdateCDNContainerOptions opts = new UpdateCDNContainerOptions();
> + opts.ttl(1234567);
> + opts.logRetention(true);
> + opts.enabled(false);
> +
> + // update the container
> + assertTrue(api.cdnApiInRegion(regionId).update(name, opts));
> +
> + CDNContainer container = api.cdnApiInRegion(regionId).get(name);
> + assertEquals(container.getTtl(), 1234567);
> + assertTrue(container.isLogRetentionEnabled());
> + assertFalse(container.isEnabled());
We're testing an update but verifying that now of the values change? I guess that's the intention, but then perhaps rename the test?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/85/files#r10955929