You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Christopher Dancy <no...@github.com> on 2015/04/11 18:56:25 UTC
[jclouds-labs] JCLOUDS-886: Implementation of Roles API (#166)
Initial impl of Roles API.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/166
-- Commit Summary --
* Initial implementation of Roles API
-- File Changes --
M shipyard/src/main/java/org/jclouds/shipyard/ShipyardApi.java (4)
A shipyard/src/main/java/org/jclouds/shipyard/domain/roles/RoleInfo.java (39)
M shipyard/src/main/java/org/jclouds/shipyard/fallbacks/ShipyardFallbacks.java (11)
A shipyard/src/main/java/org/jclouds/shipyard/features/RolesApi.java (61)
M shipyard/src/main/java/org/jclouds/shipyard/handlers/ShipyardErrorHandler.java (3)
A shipyard/src/test/java/org/jclouds/shipyard/features/RolesApiLiveTest.java (75)
A shipyard/src/test/java/org/jclouds/shipyard/features/RolesApiMockTest.java (111)
A shipyard/src/test/resources/role-delete-create.json (3)
A shipyard/src/test/resources/role.json (4)
A shipyard/src/test/resources/roles.json (10)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/166.patch
https://github.com/jclouds/jclouds-labs/pull/166.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Christopher Dancy <no...@github.com>.
@nacx failure here is due to docker CME not this PR. I'm not entirely sure what to do about it as all code on my end is up-to-date. @andreaturli any thoughts? The line in question in labs-docker seems to be pretty straightforward:
ContainerApi api = api(DockerApi.class, server.getUrl("/").toString()).getContainerApi();
On another note: I've got 2 PR's in the pipeline: this and the Accounts API here:
https://github.com/jclouds/jclouds-labs/pull/165
@nacx please take a look at this first and merge when appropriate BEFORE PR 165.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166#issuecomment-91882306
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Christopher Dancy <no...@github.com>.
@nacx @andreaturli looks to be a flaky test after all as the most recent build was successful:
https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/681/
Not sure if it's worth the extra investigation or not...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166#issuecomment-91890845
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Ignasi Barrera <no...@github.com>.
> +import static org.testng.Assert.assertNotNull;
> +
> +import org.jclouds.shipyard.domain.roles.RoleInfo;
> +import org.jclouds.shipyard.internal.BaseShipyardApiLiveTest;
> +import org.testng.annotations.AfterClass;
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "live", testName = "RolesApiLiveTest", singleThreaded = true)
> +public class RolesApiLiveTest extends BaseShipyardApiLiveTest {
> +
> + private final String roleName = "jclouds-shipyard-test-" + UUID.randomUUID().toString().replaceAll("-", "");
> +
> + @AfterClass (alwaysRun = true)
> + protected void tearDown() {
> + boolean removed = api().deleteRole(roleName);
> + assertTrue(removed);
This will fail if the `testCreateRole` test fails. Better leave the assertions for the tests, not the tearDown methods.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166/files#r28408958
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Christopher Dancy <no...@github.com>.
> + RoleInfo role = api().getRole(roleName);
> + assertNotNull(role, "Role was not set");
> + assertTrue(role.name().equals(roleName), "Found Role name does not match expected name: found=" + role.name() + ", expected=" + roleName);
> + }
> +
> + @Test (dependsOnMethods = "testGetRole")
> + public void testListRoles() throws Exception {
> + List<RoleInfo> possibleRoles = api().listRoles();
> + assertNotNull(possibleRoles, "possibleRoles was not set");
> + assertTrue(possibleRoles.size() > 0, "Expected at least 1 Role but list was empty");
> + boolean roleFound = false;
> + for (RoleInfo possibleRole : possibleRoles) {
> + if (possibleRole.name().equals(roleName)) {
> + roleFound = true;
> + }
> + }
WTF?!? It appears that in my hazy cloud of sickness I'm currently entertaining that I made the changes to the ServiceKeys API instead of the Roles API. Still needed to be done there so that's a plus, but my apologies for seemingly completely ignoring all your comments that I thought I was addressing. I'll hop on those now ;)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166/files#r28508511
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @cdancy! Looks good now. I've just added one minor comment I've just seen. Mind addressing it and squash the commits so I can merge it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166#issuecomment-94411432
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Ignasi Barrera <no...@github.com>.
> + RoleInfo role = api().getRole(roleName);
> + assertNotNull(role, "Role was not set");
> + assertTrue(role.name().equals(roleName), "Found Role name does not match expected name: found=" + role.name() + ", expected=" + roleName);
> + }
> +
> + @Test (dependsOnMethods = "testGetRole")
> + public void testListRoles() throws Exception {
> + List<RoleInfo> possibleRoles = api().listRoles();
> + assertNotNull(possibleRoles, "possibleRoles was not set");
> + assertTrue(possibleRoles.size() > 0, "Expected at least 1 Role but list was empty");
> + boolean roleFound = false;
> + for (RoleInfo possibleRole : possibleRoles) {
> + if (possibleRole.name().equals(roleName)) {
> + roleFound = true;
> + }
> + }
Use a more idiomatic Iterables.find instead
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166/files#r28408990
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/32478ef6) and [1.9.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/e7fb5098). Thanks @cdancy!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166#issuecomment-95539041
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Ignasi Barrera <no...@github.com>.
> +import static org.testng.Assert.assertNotNull;
> +
> +import org.jclouds.shipyard.domain.roles.RoleInfo;
> +import org.jclouds.shipyard.internal.BaseShipyardApiLiveTest;
> +import org.testng.annotations.AfterClass;
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "live", testName = "RolesApiLiveTest", singleThreaded = true)
> +public class RolesApiLiveTest extends BaseShipyardApiLiveTest {
> +
> + private final String roleName = "jclouds-shipyard-test-" + UUID.randomUUID().toString().replaceAll("-", "");
> +
> + @AfterClass (alwaysRun = true)
> + protected void tearDown() {
> + boolean removed = api().deleteRole(roleName);
> + assertTrue(removed);
This still needs to be fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166/files#r28494069
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Christopher Dancy <no...@github.com>.
@nacx very awesome and thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166#issuecomment-95581166
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Christopher Dancy <no...@github.com>.
@nacx all comments should be addressed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166#issuecomment-93610419
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Ignasi Barrera <no...@github.com>.
> + List<RoleInfo> listRoles();
> +
> + @Named("roles:info")
> + @Fallback(NullOnRoleNotFoundAnd500.class)
> + @GET
> + @Path("/{name}")
> + RoleInfo getRole(@PathParam("name") String name);
> +
> + @Named("roles:create")
> + @POST
> + void createRole(@WrapWith("name") String name);
> +
> + @Named("roles:delete")
> + @Fallback(BooleanOnRoleNotFoundAnd500.class)
> + @DELETE
> + Boolean deleteRole(@WrapWith("name") String name);
Can we better return a primitive `boolean` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166/files#r28676216
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Christopher Dancy <no...@github.com>.
> }
> - }
> - assertTrue(serviceKeyFound, "Expected but could not find ServiceKey amongst " + possibleServiceKeys.size() + " found");
Maybe instead just use the overloaded version and supply the default "null" value? I think that would make things more readable IMO and is what I intended to do originally.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166/files#r28509117
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Ignasi Barrera <no...@github.com>.
> }
> - }
> - assertTrue(serviceKeyFound, "Expected but could not find ServiceKey amongst " + possibleServiceKeys.size() + " found");
The `find` method will fail if it does not find any result. Use instead:
```java
assertTrue(any(possibleServiceKeys, new Predicate<ServiceKey>() {
@Override
public boolean apply(ServiceKey input) {
return input.key().equals(serviceKey);
}
}, "Expected but could not find ServiceKey amongst " + possibleServiceKeys.size() + " found");
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166/files#r28494058
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Ignasi Barrera <no...@github.com>.
> + RoleInfo role = api().getRole(roleName);
> + assertNotNull(role, "Role was not set");
> + assertTrue(role.name().equals(roleName), "Found Role name does not match expected name: found=" + role.name() + ", expected=" + roleName);
> + }
> +
> + @Test (dependsOnMethods = "testGetRole")
> + public void testListRoles() throws Exception {
> + List<RoleInfo> possibleRoles = api().listRoles();
> + assertNotNull(possibleRoles, "possibleRoles was not set");
> + assertTrue(possibleRoles.size() > 0, "Expected at least 1 Role but list was empty");
> + boolean roleFound = false;
> + for (RoleInfo possibleRole : possibleRoles) {
> + if (possibleRole.name().equals(roleName)) {
> + roleFound = true;
> + }
> + }
Still pending to change this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166/files#r28494106
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Christopher Dancy <no...@github.com>.
@nacx fixed minor edit, squashed and rebased, and latest cloudbees build looks good. I think we're a go.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166#issuecomment-94492625
Re: [jclouds-labs] JCLOUDS-886: Roles API implementation (#166)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public void testDeleteRole() throws Exception {
> + MockWebServer server = mockShipyardWebServer();
> + server.enqueue(new MockResponse().setResponseCode(204));
> + ShipyardApi shipyardApi = api(server.getUrl("/"));
> + RolesApi api = shipyardApi.rolesApi();
> + try {
> + boolean removed = api.deleteRole("admin");
> + assertTrue(removed);
> + assertSent(server, "DELETE", "/api/roles", new String(payloadFromResource("/role-delete-create.json")));
> + } finally {
> + shipyardApi.close();
> + server.shutdown();
> + }
> + }
> +}
Add the mock tests that exercise the fallbacks.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/166/files#r28409163