You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by alibazlamit <no...@github.com> on 2016/12/12 18:54:56 UTC
[jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
@nacx the Users PR is ready for review, i had one issue with this specific PR since the API does not allow to use many of the endpoints that affect the user info unless its the same user that is being used to make the request. all these tests are commented out now.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/336
-- Commit Summary --
* JCLOUDS-1216 - oneandone-user-api
-- File Changes --
M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/OneAndOneApi.java (8)
A oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/GenericRequest.java (66)
M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/Status.java (68)
M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/Types.java (7)
A oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/User.java (584)
A oneandone/src/main/java/org/apache/jclouds/oneandone/rest/features/UserApi.java (127)
A oneandone/src/test/java/org/apache/jclouds/oneandone/rest/features/UserApiLiveTest.java (162)
A oneandone/src/test/java/org/apache/jclouds/oneandone/rest/features/UserApiMockTest.java (337)
A oneandone/src/test/resources/sharedStorage/get.json (24)
A oneandone/src/test/resources/sharedStorage/list.access.json (24)
A oneandone/src/test/resources/sharedStorage/list.json (62)
A oneandone/src/test/resources/sharedStorage/list.options.json (62)
A oneandone/src/test/resources/sharedStorage/server.get.json (5)
A oneandone/src/test/resources/sharedStorage/servers.list.json (12)
A oneandone/src/test/resources/user/get.api.json (7)
A oneandone/src/test/resources/user/get.api.key.json (3)
A oneandone/src/test/resources/user/get.json (18)
A oneandone/src/test/resources/user/get.permissions.json (133)
A oneandone/src/test/resources/user/list.ips.json (5)
A oneandone/src/test/resources/user/list.json (39)
A oneandone/src/test/resources/user/list.options.json (39)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/336.patch
https://github.com/jclouds/jclouds-labs/pull/336.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + public void testGet() {
+ User result = userApi().get(currentUser.id());
+
+ assertEquals(result.id(), currentUser.id());
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testUpdate() throws InterruptedException {
+ //this test will fail too since its not allowed to update other users from the API
+// String updatedDescription = "updatejclouds";
+//
+// User updateResult = userApi().update(currentUser.id(), User.UpdateUser.builder()
+// .description(updatedDescription)
+// .build());
+//
+// assertEquals(updateResult.description(), updatedDescription);
Hmmm I don't really know. The questions to answer are:
* Why would we have tests we won't be able to run?
* Under which circumstances could we run a complete test suite for this API? What preconditions should be met?
* Can we dynamically configure those preconditions so we can run the test suite?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + public void testGet() {
+ User result = userApi().get(currentUser.id());
+
+ assertEquals(result.id(), currentUser.id());
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testUpdate() throws InterruptedException {
+ //this test will fail too since its not allowed to update other users from the API
+// String updatedDescription = "updatejclouds";
+//
+// User updateResult = userApi().update(currentUser.id(), User.UpdateUser.builder()
+// .description(updatedDescription)
+// .build());
+//
+// assertEquals(updateResult.description(), updatedDescription);
I agree this should not be our highest priority.
Moving forward, do you think the existing APIs cover the funcionality to implement the ComputeService abstraction?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by alibazlamit <no...@github.com>.
alibazlamit commented on this pull request.
> + public void testGet() {
+ User result = userApi().get(currentUser.id());
+
+ assertEquals(result.id(), currentUser.id());
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testUpdate() throws InterruptedException {
+ //this test will fail too since its not allowed to update other users from the API
+// String updatedDescription = "updatejclouds";
+//
+// User updateResult = userApi().update(currentUser.id(), User.UpdateUser.builder()
+// .description(updatedDescription)
+// .build());
+//
+// assertEquals(updateResult.description(), updatedDescription);
we could but many of those test will break the flow since it either blocks IP's, disable the user or change the API key, how would you like to proceed with this?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by alibazlamit <no...@github.com>.
alibazlamit commented on this pull request.
> + public void testGet() {
+ User result = userApi().get(currentUser.id());
+
+ assertEquals(result.id(), currentUser.id());
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testUpdate() throws InterruptedException {
+ //this test will fail too since its not allowed to update other users from the API
+// String updatedDescription = "updatejclouds";
+//
+// User updateResult = userApi().update(currentUser.id(), User.UpdateUser.builder()
+// .description(updatedDescription)
+// .build());
+//
+// assertEquals(updateResult.description(), updatedDescription);
I do believe so, we have covered all the important server operations, I will delete this PR and start a new one for the compute api.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + public void testGet() {
+ User result = userApi().get(currentUser.id());
+
+ assertEquals(result.id(), currentUser.id());
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testUpdate() throws InterruptedException {
+ //this test will fail too since its not allowed to update other users from the API
+// String updatedDescription = "updatejclouds";
+//
+// User updateResult = userApi().update(currentUser.id(), User.UpdateUser.builder()
+// .description(updatedDescription)
+// .build());
+//
+// assertEquals(updateResult.description(), updatedDescription);
That sounds great. Thanks! :)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by Ignasi Barrera <no...@github.com>.
nacx requested changes on this pull request.
Thanks @alibazlamit!
> +
+@AutoValue
+public abstract class User {
+
+ public abstract String id();
+
+ public abstract String name();
+
+ @Nullable
+ public abstract String description();
+
+ @Nullable
+ public abstract String email();
+
+ @Nullable
+ public abstract String state();
Should this be of type `UserState`?
> + public abstract Builder email(String email);
+
+ public abstract Builder password(String password);
+
+ public abstract UpdateUser build();
+ }
+ }
+
+ @AutoValue
+ public abstract static class AddUserIp {
+
+ public abstract List<String> ips();
+
+ @SerializedNames({"ips"})
+ public static AddUserIp create(List<String> ips) {
+ return new AutoValue_User_AddUserIp(ips == null ? ImmutableList.<String>of() : ips);
Enforce an immutable list if the parameter is not null.
> +
+ @AutoValue
+ public abstract static class AddUserIp {
+
+ public abstract List<String> ips();
+
+ @SerializedNames({"ips"})
+ public static AddUserIp create(List<String> ips) {
+ return new AutoValue_User_AddUserIp(ips == null ? ImmutableList.<String>of() : ips);
+ }
+ }
+
+ @AutoValue
+ public abstract static class Api {
+
+ public abstract Boolean active();
Prefer primitive types for non-nullable fields.
> + }
+
+ @AutoValue
+ public abstract static class Api {
+
+ public abstract Boolean active();
+
+ @Nullable
+ public abstract String key();
+
+ @Nullable
+ public abstract List<String> allowedIps();
+
+ @SerializedNames({"active", "key", "allowed_ips"})
+ public static Api create(Boolean active, String key, List<String> allowedIps) {
+ return new AutoValue_User_Api(active, key, allowedIps == null ? ImmutableList.<String>of() : allowedIps);
Immutable when not null.
> +
+ @Nullable
+ public abstract String key();
+
+ @Nullable
+ public abstract List<String> allowedIps();
+
+ @SerializedNames({"active", "key", "allowed_ips"})
+ public static Api create(Boolean active, String key, List<String> allowedIps) {
+ return new AutoValue_User_Api(active, key, allowedIps == null ? ImmutableList.<String>of() : allowedIps);
+ }
+
+ @AutoValue
+ public abstract static class UpdateApi {
+
+ public abstract Boolean active();
Same about primitives.
> +import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.jclouds.oneandone.rest.domain.User;
+import org.apache.jclouds.oneandone.rest.domain.options.GenericQueryOptions;
+import org.apache.jclouds.oneandone.rest.filters.AuthenticateRequest;
+import org.jclouds.Fallbacks;
+import org.jclouds.rest.annotations.BinderParam;
+import org.jclouds.rest.annotations.Fallback;
+import org.jclouds.rest.annotations.MapBinder;
+import org.jclouds.rest.annotations.RequestFilters;
+import org.jclouds.rest.annotations.SelectJson;
+import org.jclouds.rest.binders.BindToJsonPayload;
+
+@Path("/users")
+@Produces("application/json")
Remove this and leave only the method specific ones
> + public void testGet() {
+ User result = userApi().get(currentUser.id());
+
+ assertEquals(result.id(), currentUser.id());
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testUpdate() throws InterruptedException {
+ //this test will fail too since its not allowed to update other users from the API
+// String updatedDescription = "updatejclouds";
+//
+// User updateResult = userApi().update(currentUser.id(), User.UpdateUser.builder()
+// .description(updatedDescription)
+// .build());
+//
+// assertEquals(updateResult.description(), updatedDescription);
Can't we configure these tests to update the logged user? Could create a new API context for the just created user and use that to implement these tests?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336#pullrequestreview-12683948
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by alibazlamit <no...@github.com>.
Closed #336.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336#event-912664936
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by alibazlamit <no...@github.com>.
@alibazlamit pushed 1 commit.
7f26631 Changed eol
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336/files/54e42c64e745738f897d0cd7fa010e2b8c7d7526..7f266318b5371f2d275add0d6c709f8c32be8b46
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by alibazlamit <no...@github.com>.
alibazlamit commented on this pull request.
> +import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.jclouds.oneandone.rest.domain.User;
+import org.apache.jclouds.oneandone.rest.domain.options.GenericQueryOptions;
+import org.apache.jclouds.oneandone.rest.filters.AuthenticateRequest;
+import org.jclouds.Fallbacks;
+import org.jclouds.rest.annotations.BinderParam;
+import org.jclouds.rest.annotations.Fallback;
+import org.jclouds.rest.annotations.MapBinder;
+import org.jclouds.rest.annotations.RequestFilters;
+import org.jclouds.rest.annotations.SelectJson;
+import org.jclouds.rest.binders.BindToJsonPayload;
+
+@Path("/users")
+@Produces("application/json")
`@Produces("application/json")` we have been using this approach so far not a method specific tag, you recommended this approach in the beginning.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> +import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.jclouds.oneandone.rest.domain.User;
+import org.apache.jclouds.oneandone.rest.domain.options.GenericQueryOptions;
+import org.apache.jclouds.oneandone.rest.filters.AuthenticateRequest;
+import org.jclouds.Fallbacks;
+import org.jclouds.rest.annotations.BinderParam;
+import org.jclouds.rest.annotations.Fallback;
+import org.jclouds.rest.annotations.MapBinder;
+import org.jclouds.rest.annotations.RequestFilters;
+import org.jclouds.rest.annotations.SelectJson;
+import org.jclouds.rest.binders.BindToJsonPayload;
+
+@Path("/users")
+@Produces("application/json")
Hmmm I think there was some misunderstanding then. We should only add the "content-type" headers in the requests that generate a body, and that's what that annotation does. If we put it at class level, all requests will have it, even GET requests without body. I don't know if the API needs it, but we'd better put it just where needed.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by alibazlamit <no...@github.com>.
I do believe so, we have covered all the important server operations, I will delete this PR and start a new one for the compute api.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336#issuecomment-269352805
Re: [jclouds/jclouds-labs] JCLOUDS-1216 - oneandone-user-api (#336)
Posted by alibazlamit <no...@github.com>.
alibazlamit commented on this pull request.
> + public void testGet() {
+ User result = userApi().get(currentUser.id());
+
+ assertEquals(result.id(), currentUser.id());
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testUpdate() throws InterruptedException {
+ //this test will fail too since its not allowed to update other users from the API
+// String updatedDescription = "updatejclouds";
+//
+// User updateResult = userApi().update(currentUser.id(), User.UpdateUser.builder()
+// .description(updatedDescription)
+// .build());
+//
+// assertEquals(updateResult.description(), updatedDescription);
@nacx
I think that this PR and the Users API does not make a lot of sense to jClouds, most of the operations are only allowed to the current user, and changing the API key or deleting the users might break any flow, if you agree i think this feature could be skipped for jClouds, WDYT?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/336