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 17:50:41 UTC
[jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Initial accounts implementation. Mock tests and Live tests have both been run locally and verified working.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/165
-- Commit Summary --
* Accounts API implementation
-- File Changes --
M shipyard/src/main/java/org/jclouds/shipyard/ShipyardApi.java (4)
A shipyard/src/main/java/org/jclouds/shipyard/domain/accounts/AccountInfo.java (42)
A shipyard/src/main/java/org/jclouds/shipyard/domain/accounts/CreateAccount.java (45)
A shipyard/src/main/java/org/jclouds/shipyard/domain/roles/CreateRole.java (35)
A shipyard/src/main/java/org/jclouds/shipyard/domain/roles/RoleInfo.java (38)
M shipyard/src/main/java/org/jclouds/shipyard/fallbacks/ShipyardFallbacks.java (11)
A shipyard/src/main/java/org/jclouds/shipyard/features/AccountsApi.java (56)
M shipyard/src/main/java/org/jclouds/shipyard/handlers/ShipyardErrorHandler.java (5)
A shipyard/src/test/java/org/jclouds/shipyard/features/AccountsApiLiveTest.java (70)
A shipyard/src/test/java/org/jclouds/shipyard/features/AccountsApiMockTest.java (94)
A shipyard/src/test/resources/account-create.json (7)
A shipyard/src/test/resources/account-delete.json (3)
A shipyard/src/test/resources/accounts.json (18)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/165.patch
https://github.com/jclouds/jclouds-labs/pull/165.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract String password();
> +
> + public abstract RoleInfo role();
> +
> + CreateAccount() {
> + }
> +
> + @SerializedNames({ "username", "password", "role"})
> + public static CreateAccount create(String username, String password, RoleInfo role) {
> + checkArgument(username.length() > 0, "username must not be empty string");
> + checkArgument(password.length() > 0, "password must not be empty string");
> + if (role == null) role = RoleInfo.create("user", null);
> + return new AutoValue_CreateAccount(username, password, role);
> + }
> +
> + @SerializedNames({ "username", "password", "role"})
Remove the `role` fro the annotation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165/files#r29661025
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract RoleInfo role();
> +
> + CreateAccount() {
> + }
> +
> + @SerializedNames({ "username", "password", "role"})
> + public static CreateAccount create(String username, String password, RoleInfo role) {
> + checkArgument(username.length() > 0, "username must not be empty string");
> + checkArgument(password.length() > 0, "password must not be empty string");
> + if (role == null) role = RoleInfo.create("user", null);
> + return new AutoValue_CreateAccount(username, password, role);
> + }
> +
> + @SerializedNames({ "username", "password" })
> + public static CreateAccount create(String username, String password) {
> + return CreateAccount.create(username, password, null);
In my last comment I suggested to try removing the annotation from this method, leaving only one method with the `@SerializedNames` annotation in the class. Could you try that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165/files#r30278205
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
Ok @cdancy. Thanks for all the good work you've put in this provider!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165#issuecomment-125109191
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @cdancy!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165#issuecomment-93317048
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
> +public abstract class CreateAccount {
> +
> + public abstract String username();
> +
> + public abstract String password();
> +
> + public abstract CreateRole role();
> +
> + CreateAccount() {
> + }
> +
> + @SerializedNames({ "username", "password", "role"})
> + public static CreateAccount create(String username, String password, CreateRole role) {
> + checkArgument(username.length() > 0, "username must not be empty string");
> + checkArgument(password.length() > 0, "password must not be empty string");
> + if (role == null) role = CreateRole.create("user");
Instead of doing this I'dd add two factory methods, one with the role and one without it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165/files#r28409331
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Christopher Dancy <no...@github.com>.
> + public abstract String password();
> +
> + public abstract RoleInfo role();
> +
> + CreateAccount() {
> + }
> +
> + @SerializedNames({ "username", "password", "role"})
> + public static CreateAccount create(String username, String password, RoleInfo role) {
> + checkArgument(username.length() > 0, "username must not be empty string");
> + checkArgument(password.length() > 0, "password must not be empty string");
> + if (role == null) role = RoleInfo.create("user", null);
> + return new AutoValue_CreateAccount(username, password, role);
> + }
> +
> + @SerializedNames({ "username", "password", "role"})
That appears to be required which is why I left it in ;) Removing it causes jclouds to complain about it not being there
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165/files#r29667322
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Christopher Dancy <no...@github.com>.
@nacx comments have been addressed and things look good on my end. Now that JCLOUDS-886 has been merged we are good to go here. This is the last piece of work I wanted to get in before tackling the compute API.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165#issuecomment-98151917
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Christopher Dancy <no...@github.com>.
> + public abstract RoleInfo role();
> +
> + CreateAccount() {
> + }
> +
> + @SerializedNames({ "username", "password", "role"})
> + public static CreateAccount create(String username, String password, RoleInfo role) {
> + checkArgument(username.length() > 0, "username must not be empty string");
> + checkArgument(password.length() > 0, "password must not be empty string");
> + if (role == null) role = RoleInfo.create("user", null);
> + return new AutoValue_CreateAccount(username, password, role);
> + }
> +
> + @SerializedNames({ "username", "password" })
> + public static CreateAccount create(String username, String password) {
> + return CreateAccount.create(username, password, null);
@nacx this fails going through cloudbees but succeeds locally for me. Not exactly sure why...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165/files#r30035492
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
Just one very minor. Mind addressing it and squash the commits to we can cleanly merge this? Thanks @cdancy!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165#issuecomment-99050831
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Christopher Dancy <no...@github.com>.
@nacx we can remove this PR. Project is more/less dead and I've just submitted a PR to remove it from jclouds-labs.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165#issuecomment-124882951
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract String password();
> +
> + public abstract RoleInfo role();
> +
> + CreateAccount() {
> + }
> +
> + @SerializedNames({ "username", "password", "role"})
> + public static CreateAccount create(String username, String password, RoleInfo role) {
> + checkArgument(username.length() > 0, "username must not be empty string");
> + checkArgument(password.length() > 0, "password must not be empty string");
> + if (role == null) role = RoleInfo.create("user", null);
> + return new AutoValue_CreateAccount(username, password, role);
> + }
> +
> + @SerializedNames({ "username", "password", "role"})
Oh, you're right. It might fail [here](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/json/internal/NamingStrategies.java#L177-193).
jclouds uses that annotation to get the serialized names for the fields, and matches the declared fields in the class with the annotation values, expecting all fields to have the corresponding value. As having the extra "role" parameter in the annotation looks confusing, can you try just removing the annotation from this method, and leave it only in the factory method that accepts all the parameters? This would be more close to the approach we follow with the constructors, where we only annotate the constructor that has all values.
I haven't tried it with the factory method approach, but looking at the code I think it should work.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165/files#r29838864
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.shipyard.domain.accounts.AccountInfo;
> +import org.jclouds.shipyard.domain.accounts.CreateAccount;
> +import org.jclouds.shipyard.internal.BaseShipyardApiLiveTest;
> +import org.testng.annotations.AfterClass;
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "live", testName = "AccountsApiLiveTest", singleThreaded = true)
> +public class AccountsApiLiveTest extends BaseShipyardApiLiveTest {
> +
> + private final String username = UUID.randomUUID().toString().replaceAll("-", "");
> + private final String password = UUID.randomUUID().toString().replaceAll("-", "");
> +
> + @AfterClass (alwaysRun = true)
> + protected void tearDown() {
> + boolean removed = api().deleteAccount(username);
> + assertTrue(removed);
This will fail if the test that creates the account fails.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165/files#r28409388
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Christopher Dancy <no...@github.com>.
@nacx let's please merge PR 166 before this as the Accounts API has a minor dependency on the Roles API. Both are fairly simple and straight forward so I don't imagine too much churn.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165#issuecomment-91882598
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public void testCreateAccount() throws Exception {
> + api().createAccount(CreateAccount.create(username, password, null));
> + }
> +
> + @Test (dependsOnMethods = "testCreateAccount")
> + public void testListAccounts() throws Exception {
> + List<AccountInfo> possibleAccounts = api().listAccounts();
> + assertNotNull(possibleAccounts, "possibleAccounts was not set");
> + assertTrue(possibleAccounts.size() > 0, "Expected at least 1 Account but list was empty");
> + boolean accountFound = false;
> + for (AccountInfo possibleAccount : possibleAccounts) {
> + if (possibleAccount.username().equals(username)) {
> + accountFound = true;
> + }
> + }
Use a more idiomatic Iterables.find or similar approach.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165/files#r28409402
Re: [jclouds-labs] JCLOUDS-882: Accounts API implementation (#165)
Posted by Ignasi Barrera <no...@github.com>.
Closed #165.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/165#event-365790610