You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by jasminSPC <no...@github.com> on 2015/12/11 15:02:53 UTC
[jclouds-labs] Profitbricks REST - Datacenter (#225)
Thanks @mirza-spc
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/225
-- Commit Summary --
* Adding Profitbricks REST API module
* artifactid rename
* Base framework for the PB REST
* Mapping PB OBjects
* Mapping PB OBjects
* Setting up REST infrastructure
* Implemented DataCenter Api
-- File Changes --
M docker/src/main/java/org/jclouds/docker/domain/Port.java (7)
M docker/src/test/java/org/jclouds/docker/parse/ContainersParseTest.java (16)
M docker/src/test/resources/containers.json (16)
M pom.xml (1)
A profitbricks-rest/.gitattributes (2)
A profitbricks-rest/pom.xml (152)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/ProfitBricksApi.java (30)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/ProfitBricksApiMetadata.java (86)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/ProfitBricksProviderMetadata.java (73)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/config/ProfitBricksComputeProperties.java (32)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/AvailabilityZone.java (32)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/DataCenter.java (81)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/DataCenters.java (40)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/FirewallRule.java (79)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/FirewallRules.java (41)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Lan.java (71)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Lans.java (40)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/LicenceType.java (31)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Loadbalancer.java (61)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Loadbalancers.java (40)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Location.java (51)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Metadata.java (41)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Nic.java (85)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Nics.java (40)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Server.java (87)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Servers.java (41)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/State.java (28)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/VMState.java (28)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Volume.java (104)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Volumes.java (40)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/options/DepthOptions.java (30)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/features/DataCenterApi.java (158)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/ProfitBricksProviderMetadataTest.java (29)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/DataCenterApiLiveTest.java (112)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/DataCenterApiMockTest.java (115)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/internal/BaseProfitBricksApiMockTest.java (139)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/internal/BaseProfitBricksLiveTest.java (42)
A profitbricks-rest/src/test/resources/datacenter/get-depth-1.json (123)
A profitbricks-rest/src/test/resources/datacenter/get-depth-2.json (553)
A profitbricks-rest/src/test/resources/datacenter/get-depth-3.json (798)
A profitbricks-rest/src/test/resources/datacenter/get-depth-4.json (1316)
A profitbricks-rest/src/test/resources/datacenter/get-depth-5.json (1330)
A profitbricks-rest/src/test/resources/datacenter/get.json (41)
A profitbricks-rest/src/test/resources/datacenter/list.json (130)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/225.patch
https://github.com/jclouds/jclouds-labs/pull/225.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + /*properties.put("oauth.endpoint", "https://cloud.digitalocean.com/v1/oauth/token");
> + properties.put(JWS_ALG, "RS256");
> + properties.put(AUDIENCE, "https://cloud.digitalocean.com/v1/oauth/token");
> + properties.put(CREDENTIAL_TYPE, BEARER_TOKEN_CREDENTIALS.toString());
> + properties.put(PROPERTY_SESSION_INTERVAL, 3600);
> + properties.put(TEMPLATE, "osFamily=UBUNTU,os64Bit=true");*/
> + properties.put(POLL_INITIAL_PERIOD, 5000);
> + properties.put(POLL_MAX_PERIOD, 20000);
> + // Node operations in DigitalOcean can be quite slow. Use a 5 minutes
> + // timeout by default
> + properties.put(TIMEOUT_NODE_RUNNING, 300000);
> + properties.put(TIMEOUT_NODE_SUSPENDED, 300000);
> + properties.put(TIMEOUT_NODE_TERMINATED, 300000);
Also remove these defaults if they don't apply to this provider.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47700602
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-165504455
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + private final Set<Module> modules = ImmutableSet.<Module>of(new ExecutorServiceModule(sameThreadExecutor()));
> +
> + protected MockWebServer server;
> + protected ProfitBricksApi api;
> + private Json json;
> +
> + // So that we can ignore formatting.
> + private final JsonParser parser = new JsonParser();
> +
> + @BeforeMethod
> + public void start() throws IOException {
> + server = new MockWebServer();
> + server.play();
> + ApiContext<ProfitBricksApi> ctx = ContextBuilder.newBuilder("profitbricks-rest")
> + .credentials("", MOCK_BEARER_TOKEN)
The provider uses basic auth. Use appropriate mock values; not an oauth token.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47705189
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.apache.jclouds.profitbricks.rest.domain;
> +
> +public enum LicenceType {
> +
> + WINDOWS, LINUX, OTHER, UNRECOGNIZED;
> +
> + public static LicenceType fromValue(String v) {
> + try {
> + return valueOf(v);
> + } catch (IllegalArgumentException ex) {
> + return UNRECOGNIZED;
> + }
Use the same approach that in the other enums.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r48140565
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + properties.put(POLL_MAX_PERIOD, 20000);
> + // Node operations in DigitalOcean can be quite slow. Use a 5 minutes
> + // timeout by default
> + properties.put(TIMEOUT_NODE_RUNNING, 300000);
> + properties.put(TIMEOUT_NODE_SUSPENDED, 300000);
> + properties.put(TIMEOUT_NODE_TERMINATED, 300000);
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<ProfitBricksApi, Builder> {
> +
> + protected Builder() {
> + id("profitbricks-rest")
> + .name("Digital Ocean v2 API")
> + .identityName("Not used for OAuth")
> + .credentialName("Must be oauth2 Bearer Token")
Change these literals accordingly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47700734
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract String href();
> +
> + public abstract Metadata metadata();
> +
> + public abstract Properties properties();
> +
> + public enum Protocol {
> +
> + TCP, UDP, ICMP, ANY, UNRECOGNIZED;
> +
> + public static Protocol fromValue(String value) {
> + try {
> + return valueOf(value);
> + } catch (IllegalArgumentException e) {
> + return UNRECOGNIZED;
> + }
Use Enums.getIfPresent like in the previous enum or change that one, but make the code consistent using the same pattern when addressing the same thing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47701667
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-165567755
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-165520347
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "unit", testName = "DataCenterApiMockTest", singleThreaded = true)
> +public class DataCenterApiMockTest extends BaseProfitBricksApiMockTest {
> +
> + @Test
> + public void testGetList() {
> + server.enqueue(
> + new MockResponse().setBody(stringFromResource("/datacenter/list.json"))
> + );
> +
> + List<DataCenter> list = dataCenterApi().list(new DepthOptions().depth(1));
> +
> + assertNotNull(list);
> + assertEquals(list.size(), 3);
> + assertEquals(list.get(0).properties().name(), "vea");
You need to verify the recorded request. MockWebServer will return each enqueued request upon *any* request. You need to verify that the generated request is the expected one: expected request uri, method, headers and body.
The same must be applied in all methods in this class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47704683
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.json.Json;
> +import org.jclouds.rest.annotations.Fallback;
> +import org.jclouds.rest.annotations.MapBinder;
> +import org.jclouds.rest.annotations.PATCH;
> +import org.jclouds.rest.annotations.PayloadParam;
> +import org.jclouds.rest.annotations.ResponseParser;
> +import org.jclouds.rest.annotations.SelectJson;
> +import org.jclouds.rest.binders.BindToJsonPayload;
> +
> +@Path("/datacenters")
> +@RequestFilters(BasicAuthentication.class)
> +public interface DataCenterApi extends Closeable {
> +
> + @Named("datacenter:list")
> + @GET
> + @Produces("application/vnd.profitbricks.resource+json")
Remove this annotation if this method is not sending this content-type, and apply the same for all methods that do not generate a body.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r48141402
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + @AutoValue
> + public abstract static class Properties {
> +
> + public abstract String name();
> +
> + public abstract Protocol protocol();
> +
> + public abstract String sourceMac();
> +
> + public abstract String sourceIp();
> +
> + public abstract String targetIp();
> +
> + public abstract String icmpCode();
> +
> + public abstract String icmpType();
Are all these properties mandatory? Will they *always* have a value in the responses?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47701768
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + <dependency>
> + <groupId>org.apache.jclouds</groupId>
> + <artifactId>jclouds-compute</artifactId>
> + <version>${jclouds.version}</version>
> + <type>test-jar</type>
> + <scope>test</scope>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-sshj</artifactId>
> + <version>${jclouds.version}</version>
> + <scope>test</scope>
> + </dependency>
> + <dependency>
> + <groupId>com.squareup.okhttp</groupId>
> + <artifactId>mockwebserver</artifactId>
Declare this dependency in test scope.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r48138263
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + public ProfitBricksApiMetadata() {
> + this(new Builder());
> + }
> +
> + protected ProfitBricksApiMetadata(Builder builder) {
> + super(builder);
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + /*properties.put("oauth.endpoint", "https://cloud.digitalocean.com/v1/oauth/token");
> + properties.put(JWS_ALG, "RS256");
> + properties.put(AUDIENCE, "https://cloud.digitalocean.com/v1/oauth/token");
> + properties.put(CREDENTIAL_TYPE, BEARER_TOKEN_CREDENTIALS.toString());
> + properties.put(PROPERTY_SESSION_INTERVAL, 3600);
> + properties.put(TEMPLATE, "osFamily=UBUNTU,os64Bit=true");*/
Remove the commented copy/pasted properties.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47700551
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + private final String id;
> +
> + Location(String id) {
> + this.id = id;
> + }
> +
> + public String value() {
> + return id;
> + }
> +
> + public static Location fromValue(String v) {
> + try {
> + return valueOf(v);
> + } catch (IllegalArgumentException ex) {
> + return UNRECOGNIZED;
> + }
Same here. Use a consistent approach in all enums.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r48140732
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + properties.setProperty(PROPERTY_REGION + ".DE.FKB." + ISO3166_CODES, "DE-FKB");
> + properties.setProperty(PROPERTY_REGION + ".DE.FRA." + ISO3166_CODES, "DE-FRA");
> + properties.setProperty(PROPERTY_REGION + ".US.LAS." + ISO3166_CODES, "US-LAS");
> + properties.setProperty(PROPERTY_REGION + ".US.LASDEV." + ISO3166_CODES, "US-LASDEV");
> + return properties;
> + }
> +
> + public static class Builder extends BaseProviderMetadata.Builder {
> +
> + protected Builder() {
> + id("profitbricks-rest")
> + .name("ProfitBricks REST Compute")
> + .apiMetadata(new ProfitBricksApiMetadata())
> + .homepage(URI.create("https://www.profitbricks.com/"))
> + .console(URI.create("https://my.profitbricks.com/dashboard/dcdr2"))
> + .iso3166Codes("DE-FKB", "DE-FRA", "US-LAS", "US-LASDEV")
Fix this according to the previous comment.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r48140453
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-165486474
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + assertTrue(list.isEmpty());
> + }
> +
> + @Test
> + public void testGetDataCenter() {
> + MockResponse response = new MockResponse();
> + response.setBody(stringFromResource("/datacenter/get.json"));
> + response.setHeader("Content-Type", "application/vnd.profitbricks.resource+json");
> +
> + server.enqueue(response);
> +
> + DataCenter dataCenter = dataCenterApi().getDataCenter("some-id");
> +
> + assertNotNull(dataCenter);
> + assertEquals(dataCenter.properties().name(), "docker");
> + }
Add a mock test for the get + 404, as you define a fallback for that case.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47704788
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +public abstract class Lan {
> +
> + public abstract String id();
> +
> + public abstract String type();
> +
> + public abstract String href();
> +
> + @Nullable
> + public abstract Metadata metadata();
> +
> + @Nullable
> + public abstract Properties properties();
> +
> + @Nullable
> + public abstract Entities entities();
Ok, Then is would make sense to mark the `Entities` properties as `@Nullable` in all objects. Otherwise deserialization will fail if the annotation is missing and the field does not come in the response.
What about the metadata and properties fields? Shouldn't be always present?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47884026
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + DataCenter dataCenter = dataCenterApi().getDataCenter("some-id");
> +
> + assertNotNull(dataCenter);
> + assertEquals(dataCenter.properties().name(), "docker");
> +
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/datacenters/some-id");
> + }
> +
> + public void testGetDataCenterWith404() throws InterruptedException {
> + server.enqueue(response404());
> +
> + DataCenter dataCenter = dataCenterApi().getDataCenter("some-id");
> +
> + assertEquals(dataCenter, null);
[minor] Use assertNull
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r48141161
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-165700676
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public ProfitBricksProviderMetadata(Builder builder) {
> + super(builder);
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = ProfitBricksApiMetadata.defaultProperties();
> + return properties;
> + }
> +
> + public static class Builder extends BaseProviderMetadata.Builder {
> +
> + protected Builder() {
> + id("profitbricks-rest")
> + .name("DigitalOcean Compute Services")
Fix this literal.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47700922
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @jasminSPC! This is a good start. Apart from the inline comments, there are two missing pieces for the initial structure of the provider:
* The creation of the ProfitBricksHttpApiModule (declare it too in the api metadata default module list).
* The creating of the ProfitBricksErrorHandler (you will configure it in the http api module) that transforms each failed response in the appropriate Java exception.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-164911578
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-164433413
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + DataCenter getDataCenter(@PathParam("id") String id);
> +
> + @Named("datacenter:get")
> + @GET
> + @Path("/{id}")
> + @ResponseParser(DataCenterParser.class)
> + @Produces("application/vnd.profitbricks.resource+json")
> + @Fallback(NullOnNotFoundOr404.class)
> + DataCenter getDataCenter(@PathParam("id") String id, DepthOptions options);
> +
> + @Named("datacenter:create")
> + @POST
> + @ResponseParser(DataCenterParser.class)
> + @Produces("application/vnd.profitbricks.resource+json")
> + @MapBinder(DataCenterCreateMapBinder.class)
> + @Fallback(NullOnNotFoundOr404.class)
Do not use 404 fallbacks on POST/PUT. Just remove the fallback and let the method throw an exception if a 404 occurs.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47703746
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +package org.apache.jclouds.profitbricks.rest.config;
> +
> +public class ProfitBricksComputeProperties {
> +
> + public static final String POLL_PREDICATE_DATACENTER = "jclouds.profitbricks.predicate.datacenter";
> + public static final String POLL_PREDICATE_SNAPSHOT = "jclouds.profitbricks.predicate.snapshot";
> +
> + public static final String POLL_TIMEOUT = "jclouds.profitbricks.poll.timeout";
> + public static final String POLL_PERIOD = "jclouds.profitbricks.operation.poll.initial-period";
> + public static final String POLL_MAX_PERIOD = "jclouds.profitbricks.operation.poll.max-period";
> +
> + private ProfitBricksComputeProperties() {
> + throw new AssertionError("Intentionally unimplemented");
> + }
> +
> +}
Remove this class until these properties are actually needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47701042
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
> + @Test
> + public void testDelete() {
> + DataCenter dataCenter = createDataCenter();
> + dataCenterApi().delete(dataCenter.id());
> + dataCenter = getDataCenter(dataCenter.id());
> + //assertEquals(dataCenter, null);
> + }
> +
> + @Test
> + public void testDepth() {
> +
> + DataCenter dataCenter = createDataCenter();
> +
> + for (int i = 1; i <= 5; ++i) {
> + dataCenter = getDataCenter(dataCenter.id(), new DepthOptions().depth(i));
> + assertNotNull(dataCenter);
Sure, but I wouldn't go to far in, as we would need to have all the sub-items instantiated such as servers, lans.. and so on. (although we can come back to it, when these other parts of API are implemented)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47895871
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +@AutoValue
> +public abstract class Loadbalancers {
> +
> + public abstract String id();
> +
> + public abstract String type();
> +
> + public abstract String href();
> +
> + public abstract List<Loadbalancer> items();
> +
> + @SerializedNames({"id", "type", "href", "items"})
> + public static Loadbalancers create(String id, String type, String href, List<Loadbalancer> items) {
> + return new AutoValue_Loadbalancers(id, type, href, items == null ? ImmutableList.<Loadbalancer>of() : copyOf(items));
> + }
> +}
Remove wrapper.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47702345
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + @Named("datacenter:list")
> + @GET
> + @Produces("application/vnd.profitbricks.resource+json")
> + @SelectJson("items")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<DataCenter> list();
> +
> + @Named("datacenter:list")
> + @GET
> + @Produces("application/vnd.profitbricks.resource+json")
> + @SelectJson("items")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<DataCenter> list(DepthOptions options);
> +
> + /**
> + * @param identifier Data Center identifier
`@param id`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47703619
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
Amended the commit message and pushed to master as [d63af3c2](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/d63af3c2).
Thanks @jasminSPC and @mirza-spc!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-167095356
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-164399785
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + @Produces("application/vnd.profitbricks.resource+json")
> + @MapBinder(DataCenterCreateMapBinder.class)
> + @Fallback(NullOnNotFoundOr404.class)
> + DataCenter create(
> + @PayloadParam("name") String name,
> + @PayloadParam("description") String description,
> + @PayloadParam("location") String location
> + );
> +
> + @Named("datacenter:update")
> + @PATCH
> + @Path("/{id}")
> + @ResponseParser(DataCenterParser.class)
> + @Produces("application/vnd.profitbricks.partial-properties+json")
> + @MapBinder(BindToJsonPayload.class)
> + @Fallback(NullOnNotFoundOr404.class)
Remove the fallback here too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47703842
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + public void testUpdate() {
> + server.enqueue(
> + new MockResponse().setBody(stringFromResource("/datacenter/get.json"))
> + );
> +
> + DataCenter dataCenter = dataCenterApi().update("some-id", "new name");
> + }
> +
> + @Test
> + public void testDelete() {
> + server.enqueue(
> + new MockResponse().setBody("")
> + );
> +
> + dataCenterApi().delete("some-id");
> + }
Add a test for the delete fallback.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47705001
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-166835073
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
I have tried to apply all the comments so far. When you are able to, you can review the changes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-165489562
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @mirza-spc! Just three small comments left!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-166863113
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract String name();
> +
> + public abstract String ip();
> +
> + public abstract boolean dhcp();
> +
> + @SerializedNames({"name", "ip", "dhcp"})
> + public static Properties create(String name, String ip, boolean dhcp) {
> + return new AutoValue_Loadbalancer_Properties(name, ip, dhcp);
> + }
> + }
> +
> + @AutoValue
> + public abstract static class Entities {
> +
> + }
Remove this class if empty.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47702263
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +package org.apache.jclouds.profitbricks.rest;
> +
> +import com.google.common.annotations.Beta;
> +import java.io.Closeable;
> +import org.apache.jclouds.profitbricks.rest.features.DataCenterApi;
> +import org.jclouds.rest.annotations.Delegate;
> +
> +@Beta
> +public interface ProfitBricksApi extends Closeable{
Style minor: Add a space before `{`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47700481
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + return dataCenterApi().getDataCenter(id);
> + }
> +
> + private DataCenter getDataCenter(String id, DepthOptions options) {
> + return dataCenterApi().getDataCenter(id, options);
> + }
> +
> + private void deleteDataCenter(String id) {
> + dataCenterApi().delete(id);
> + }
> +
> + private DataCenterApi dataCenterApi() {
> + return api.dataCenterApi();
> + }
> +
> +}
The test looks OK. Take into account that tests will take a while when the provider is complete, so you should consider chaining the tests and avoid creating a datacenter for each test if it is an expensive operation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47704471
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + DataCenter getDataCenter(@PathParam("id") String id);
> +
> + @Named("datacenter:get")
> + @GET
> + @Path("/{id}")
> + @ResponseParser(DataCenterParser.class)
> + @Produces("application/vnd.profitbricks.resource+json")
> + @Fallback(NullOnNotFoundOr404.class)
> + DataCenter getDataCenter(@PathParam("id") String id, DepthOptions options);
> +
> + @Named("datacenter:create")
> + @POST
> + @ResponseParser(DataCenterParser.class)
> + @Produces("application/vnd.profitbricks.resource+json")
> + @MapBinder(DataCenterCreateMapBinder.class)
> + @Fallback(NullOnNotFoundOr404.class)
Just POST/PUT/PATCH, because the failure is ambiguous. What does a null value mean in that case? The resource does not exist? The operation completed and the modified Object is now `null` (the response did not return a body)? These semantics with null are particularly ambiguous in POST/PUT/PATCH, so we avoid the fallbacks and let the method fail.
In DELETE we can be less restrictive, as it could make sense to return null. The object being deleted is no longer there, so there are no conceptual conflicts.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47893354
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + @Test
> + public void testDelete() {
> + DataCenter dataCenter = createDataCenter();
> + dataCenterApi().delete(dataCenter.id());
> + dataCenter = getDataCenter(dataCenter.id());
> + //assertEquals(dataCenter, null);
> + }
> +
> + @Test
> + public void testDepth() {
> +
> + DataCenter dataCenter = createDataCenter();
> +
> + for (int i = 1; i <= 5; ++i) {
> + dataCenter = getDataCenter(dataCenter.id(), new DepthOptions().depth(i));
> + assertNotNull(dataCenter);
Verify that the depth actually changes something in the returned datacenter? Verify the entities?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47704344
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + <groupId>com.squareup.okhttp</groupId>
> + <artifactId>mockwebserver</artifactId>
> + <exclusions>
> + <!-- Already provided by jclouds-sshj -->
> + <exclusion>
> + <groupId>org.bouncycastle</groupId>
> + <artifactId>bcprov-jdk15on</artifactId>
> + </exclusion>
> + </exclusions>
> + <version>2.2.0</version>
> + <type>jar</type>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-okhttp</artifactId>
> + <version>${project.parent.version}</version>
Use `jclouds.version` too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47700281
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
Thanks! I'll have a look later today/tomorrow.
For the next comments in this PR (and for upcoming new PRs), please don't squash the commits when addressing comments. It makes it a lot easier to review only the bits that have changed; otherwise we have to go through the entire PR again. We usually follow this procedure and once the PR is ready to merge, we ask to squash teh commits or squash them ourselves before merging.
NP for now :) I'm just suggesting this for the next review cycles.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-165511613
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -0,0 +1,2 @@
> +text eol=crlf
> +*.java text
Move this file to the root of the repo
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47699976
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
> +public abstract class Lan {
> +
> + public abstract String id();
> +
> + public abstract String type();
> +
> + public abstract String href();
> +
> + @Nullable
> + public abstract Metadata metadata();
> +
> + @Nullable
> + public abstract Properties properties();
> +
> + @Nullable
> + public abstract Entities entities();
My knowledge is that, whether some field is present or not, is usually controlled by the depth param. So, as you can imagine, some fields may be existent, but if depth is too shallow they may be 'null'.
So what we've done in the depth testing, is to pull a response for each of the depth levels and simply try to instantiate a DataCenter object (which contains almost all the other domain objects). If we see the test failing (because of null entry), we put nullable on the field.
Now one may argue, that the field can still be null (in the server database), and it would be correct thing to say. To check this, we can go and try to instantiate each object with null on these fields or simply check in the UI if it can be empty.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47882960
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.apache.jclouds.profitbricks.rest.domain;
> +
> +import com.google.auto.value.AutoValue;
> +import org.jclouds.json.SerializedNames;
> +
> +@AutoValue
> +public abstract class Metadata {
> +
> + public abstract String createdDate();
These were date before, but test keep failing because of a parse error:
```
java.lang.IllegalArgumentException: Error parsing data at 0
at java.text.DateFormat.parse(DateFormat.java:366)
at org.jclouds.date.internal.SimpleDateFormatDateService.cDateParse(SimpleDateFormatDateService.java:80)
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47891278
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + checkArgument(!object.isEmpty(), "The given json does not contain any object: %s", text);
> + checkArgument(object.keySet().size() == 1, "The given json does not contain more than one object: %s", text);
> + return object.get(getOnlyElement(object.keySet()));
> + }
> +
> + protected <T> T objectFromResource(String resourceName, Class<T> type) {
> + String text = stringFromResource(resourceName);
> + return json.fromJson(text, type);
> + }
> +
> + protected RecordedRequest assertSent(MockWebServer server, String method, String path) throws InterruptedException {
> + RecordedRequest request = server.takeRequest();
> + assertEquals(request.getMethod(), method);
> + assertEquals(request.getPath(), path);
> + assertEquals(request.getHeader("Accept"), "application/json");
> + assertEquals(request.getHeader("Authorization"), "Bearer " + MOCK_BEARER_TOKEN);
You will probably need to change this auth and the accept type when completing teh request verification in the mock tests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47705284
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +@Test(groups = "unit", testName = "DataCenterApiMockTest", singleThreaded = true)
> +public class DataCenterApiMockTest extends BaseProfitBricksApiMockTest {
> +
> + @Test
> + public void testGetList() {
> + server.enqueue(
> + new MockResponse().setBody(stringFromResource("/datacenter/list.json"))
> + );
> +
> + List<DataCenter> list = dataCenterApi().list(new DepthOptions().depth(1));
> +
> + assertNotNull(list);
> + assertEquals(list.size(), 3);
> + assertEquals(list.get(0).properties().name(), "vea");
> + }
Also test the list method that does not get the options parameter.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47704934
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.apache.jclouds.profitbricks.rest.domain;
> +
> +import com.google.auto.value.AutoValue;
> +import org.jclouds.json.SerializedNames;
> +
> +@AutoValue
> +public abstract class Metadata {
> +
> + public abstract String createdDate();
Change type to Date.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47702829
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +import java.util.Set;
> +import org.apache.jclouds.profitbricks.rest.ProfitBricksApi;
> +import org.apache.jclouds.profitbricks.rest.ProfitBricksProviderMetadata;
> +import org.jclouds.ContextBuilder;
> +import org.jclouds.concurrent.config.ExecutorServiceModule;
> +import org.jclouds.json.Json;
> +import org.jclouds.rest.ApiContext;
> +import static org.testng.Assert.assertEquals;
> +import org.testng.annotations.AfterMethod;
> +import org.testng.annotations.BeforeMethod;
> +import static com.google.common.base.Preconditions.checkArgument;
> +import static com.google.common.collect.Iterables.getOnlyElement;
> +
> +public class BaseProfitBricksApiMockTest {
> +
> + private static final String MOCK_BEARER_TOKEN = "c5401990f0c24135e8d6b5d260603fc71696d4738da9aa04a720229a01a2521d";
This does not apply here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47705071
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
> + return dataCenterApi().getDataCenter(id);
> + }
> +
> + private DataCenter getDataCenter(String id, DepthOptions options) {
> + return dataCenterApi().getDataCenter(id, options);
> + }
> +
> + private void deleteDataCenter(String id) {
> + dataCenterApi().delete(id);
> + }
> +
> + private DataCenterApi dataCenterApi() {
> + return api.dataCenterApi();
> + }
> +
> +}
I agree. As far as I can see, a DataCenter is purely a logical construct and does not consume any real resources. But definitely for servers, volumes and so on.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47895983
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +@AutoValue
> +public abstract class Lans {
> +
> + public abstract String id();
> +
> + public abstract String type();
> +
> + public abstract String href();
> +
> + public abstract List<Lan> items();
> +
> + @SerializedNames({"id", "type", "href", "items"})
> + public static Lans create(String id, String type, String href, List<Lan> items) {
> + return new AutoValue_Lans(id, type, href, items == null ? ImmutableList.<Lan>of() : copyOf(items));
> + }
> +}
Same about wrapper objects.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47702185
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.apache.jclouds.profitbricks.rest.domain;
> +
> +public enum Location {
> +
> + DE_FKB("de/fkb"),
> + DE_FRA("de/fra"),
> + US_LAS("us/las"),
> + US_LAS_DEV("us/lasdev"),
> + UNRECOGNIZED("unknown");
If the available locations are known in advance, they should be added to the provider metadata default properties. You can take the [Rackspace ProviderMetadata](https://github.com/jclouds/jclouds/blob/master/providers/rackspace-cloudservers-us/src/main/java/org/jclouds/rackspace/cloudservers/us/CloudServersUSProviderMetadata.java#L66-L77) as an example. Also configure the [iso codes](https://github.com/jclouds/jclouds/blob/master/providers/rackspace-cloudservers-us/src/main/java/org/jclouds/rackspace/cloudservers/us/CloudServersUSProviderMetadata.java#L103).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47702691
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
> +public abstract class Lan {
> +
> + public abstract String id();
> +
> + public abstract String type();
> +
> + public abstract String href();
> +
> + @Nullable
> + public abstract Metadata metadata();
> +
> + @Nullable
> + public abstract Properties properties();
> +
> + @Nullable
> + public abstract Entities entities();
The tree of hierarchy, with DataCenter as its root is 'filtered' by the depth level. So I think it will be always there for DataCenter, since it is the root. If however, at some point in future, there would be a grouping above datacenters (perhaps Region?), and if the depth is too low, then it could become null.
It is in a sense relative, if one is requesting an object from the api. Then the object's meta will not be null, but its childrens' meta may be, depending on the depth.
I think it would be sensible to have them nullable as well.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47887091
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> + }
> +
> + protected <T> T onlyObjectFromResource(String resourceName, TypeToken<Map<String, T>> type) {
> + // Assume JSON objects passed here will be in the form: { "entity": { ... } }
> + String text = stringFromResource(resourceName);
> + Map<String, T> object = json.fromJson(text, type.getType());
> + checkArgument(!object.isEmpty(), "The given json does not contain any object: %s", text);
> + checkArgument(object.keySet().size() == 1, "The given json does not contain more than one object: %s", text);
> + return object.get(getOnlyElement(object.keySet()));
> + }
> +
> + protected <T> T objectFromResource(String resourceName, Class<T> type) {
> + String text = stringFromResource(resourceName);
> + return json.fromJson(text, type);
> + }
Remove all protected methods that are not used.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r48141450
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +@AutoValue
> +public abstract class DataCenters {
> +
> + public abstract String id();
> +
> + public abstract String type();
> +
> + public abstract String href();
> +
> + public abstract List<DataCenter> items();
> +
> + @SerializedNames({"id", "type", "href", "items"})
> + public static DataCenters create(String id, String type, String href, List<DataCenter> items) {
> + return new AutoValue_DataCenters(id, type, href, items == null ? ImmutableList.<DataCenter>of() : copyOf(items));
> + }
> +}
I've just noticed wrappers might be needed to deserialize the "embedded entities". Is that its purpose?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47703158
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by mirza-spc <no...@github.com>.
> + DataCenter getDataCenter(@PathParam("id") String id);
> +
> + @Named("datacenter:get")
> + @GET
> + @Path("/{id}")
> + @ResponseParser(DataCenterParser.class)
> + @Produces("application/vnd.profitbricks.resource+json")
> + @Fallback(NullOnNotFoundOr404.class)
> + DataCenter getDataCenter(@PathParam("id") String id, DepthOptions options);
> +
> + @Named("datacenter:create")
> + @POST
> + @ResponseParser(DataCenterParser.class)
> + @Produces("application/vnd.profitbricks.resource+json")
> + @MapBinder(DataCenterCreateMapBinder.class)
> + @Fallback(NullOnNotFoundOr404.class)
I'm guessing this goes also for PATCH and DELETE
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47893117
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + */
> +package org.apache.jclouds.profitbricks.rest.domain;
> +
> +import com.google.auto.value.AutoValue;
> +import org.jclouds.json.SerializedNames;
> +
> +@AutoValue
> +public abstract class Metadata {
> +
> + public abstract String createdDate();
> +
> + public abstract String createdBy();
> +
> + public abstract String etag();
> +
> + public abstract String lastModifiedDate();
Change type to Date.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47702843
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +public abstract class Lan {
> +
> + public abstract String id();
> +
> + public abstract String type();
> +
> + public abstract String href();
> +
> + @Nullable
> + public abstract Metadata metadata();
> +
> + @Nullable
> + public abstract Properties properties();
> +
> + @Nullable
> + public abstract Entities entities();
These fields are not nullable in the previous classes but nullable here. It it the real behaviour of the API? Or should the corresponding `@Nullable` annotations be added to the other objects?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47702114
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
Closed #225.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#event-500117246
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + @Test
> + public void testDelete() {
> + DataCenter dataCenter = createDataCenter();
> + dataCenterApi().delete(dataCenter.id());
> + dataCenter = getDataCenter(dataCenter.id());
> + //assertEquals(dataCenter, null);
> + }
> +
> + @Test
> + public void testDepth() {
> +
> + DataCenter dataCenter = createDataCenter();
> +
> + for (int i = 1; i <= 5; ++i) {
> + dataCenter = getDataCenter(dataCenter.id(), new DepthOptions().depth(i));
> + assertNotNull(dataCenter);
Agree. There's no need for a depth 5. I'd just call depth(1) and depth(2) (or 0 and 1, whatever is cheaper) and verify the different content in the returned objects.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47909624
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-sshj</artifactId>
> + <version>${jclouds.version}</version>
> + <scope>test</scope>
> + </dependency>
> + <dependency>
> + <groupId>com.squareup.okhttp</groupId>
> + <artifactId>mockwebserver</artifactId>
> + <exclusions>
> + <!-- Already provided by jclouds-sshj -->
> + <exclusion>
> + <groupId>org.bouncycastle</groupId>
> + <artifactId>bcprov-jdk15on</artifactId>
> + </exclusion>
> + </exclusions>
> + <version>2.2.0</version>
The version should already be configured in the jclouds-project. Is it possible to remove it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47700109
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public ProfitBricksProviderMetadata() {
> + super(builder());
> + }
> +
> + public ProfitBricksProviderMetadata(Builder builder) {
> + super(builder);
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = ProfitBricksApiMetadata.defaultProperties();
> + properties.setProperty(PROPERTY_REGIONS, "DE,US");
> + properties.setProperty(PROPERTY_REGION + ".DE.FKB." + ISO3166_CODES, "DE-FKB");
> + properties.setProperty(PROPERTY_REGION + ".DE.FRA." + ISO3166_CODES, "DE-FRA");
> + properties.setProperty(PROPERTY_REGION + ".US.LAS." + ISO3166_CODES, "US-LAS");
> + properties.setProperty(PROPERTY_REGION + ".US.LASDEV." + ISO3166_CODES, "US-LASDEV");
It looks like the provider has zones and regions. I assume the region IDs are "DE" and "US", and the zone IDs are "FKB, "FRA", "LAS". The right way to configure this in jclouds is as follows:
```java
properties.setProperty(PROPERTY_ISO3166_CODES, "US-NV,DE-HE,DE-BW");
properties.setProperty(PROPERTY_REGIONS, "DE,US");
properties.setProperty(PROPERTY_REGION + ".DE.zones", "FKB,FRA")
properties.setProperty(PROPERTY_REGION + ".US.zones", "LAS");
properties.setProperty(PROPERTY_ZONES, "FKB,FRA,LAS");
properties.setProperty(PROPERTY_ZONE + ".FKB." + ISO3166_CODES, "DE-BW");
properties.setProperty(PROPERTY_ZONE + ".FRA." + ISO3166_CODES, "DE-HE");
properties.setProperty(PROPERTY_ZONE + ".LAS." + ISO3166_CODES, "US-NV");
```
Note that I've already fixed the ISO codes to use the right ones. Also, the zone IDs and regions IDs used here are up to you. Feel free to change them to something more user-friendly, as those "FKB/FRA/LAS" is what users will use as the "locationId" when choosing where they want to deploy the VMs.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r48140426
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +@AutoValue
> +public abstract class DataCenters {
> +
> + public abstract String id();
> +
> + public abstract String type();
> +
> + public abstract String href();
> +
> + public abstract List<DataCenter> items();
> +
> + @SerializedNames({"id", "type", "href", "items"})
> + public static DataCenters create(String id, String type, String href, List<DataCenter> items) {
> + return new AutoValue_DataCenters(id, type, href, items == null ? ImmutableList.<DataCenter>of() : copyOf(items));
> + }
> +}
Do the id, type and href fields provide useful information? Can we get rid of this object and let the API just "select" the items field and directly return lists of DataCenter objects? It looks unnecessary to have this entity if the fields it adds provide no useful info.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47701450
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.apache.jclouds.profitbricks.rest.domain;
> +
> +import com.google.auto.value.AutoValue;
> +import org.jclouds.json.SerializedNames;
> +
> +@AutoValue
> +public abstract class Metadata {
> +
> + public abstract String createdDate();
In which format are dates returned? This should be fixed by registering a proper date adapter in the http api module. Take a look at how [the Chef api](https://github.com/jclouds/jclouds/blob/master/apis/chef/src/main/java/org/jclouds/chef/config/ChefParserModule.java#L259) does that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47893171
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> + </dependency>
> + <dependency>
> + <groupId>com.squareup.okhttp</groupId>
> + <artifactId>mockwebserver</artifactId>
> + <exclusions>
> + <!-- Already provided by jclouds-sshj -->
> + <exclusion>
> + <groupId>org.bouncycastle</groupId>
> + <artifactId>bcprov-jdk15on</artifactId>
> + </exclusion>
> + </exclusions>
> + <type>jar</type>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-okhttp</artifactId>
Move this dependency up, "outside" the test dependencies block.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r48138306
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +public abstract class FirewallRules {
> +
> + public abstract String id();
> +
> + public abstract String type();
> +
> + public abstract String href();
> +
> + public abstract List<FirewallRule> items();
> +
> + @SerializedNames({"id", "type", "href", "items"})
> + public static FirewallRules create(String id, String type, String href, List<FirewallRule> items) {
> + return new AutoValue_FirewallRules(id, type, href, items == null ? ImmutableList.<FirewallRule>of() : copyOf(items));
> + }
> +
> +}
Same here. Consider removing this wrapper class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47701985
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
Nice update! It looks pretty good now. There are only a few minors.
Apart from that, looking at how the ProfitBricks API returns the locations (lowercase region/zone), we'll have to configure some extra bits when it comes to implement the ComputeService abstraction (as the default jclouds implementation will just use the zone id without the "region/" prefix), but that is an easy change we'll do when we get there.
Nice job!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225#issuecomment-166295135
Re: [jclouds-labs] Profitbricks REST - Datacenter (#225)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +public class BaseProfitBricksLiveTest extends BaseApiLiveTest<ProfitBricksApi> {
> +
> + public static final Location testLocation = Location.US_LAS;
> +
> + public BaseProfitBricksLiveTest() {
> + provider = "profitbricks-rest";
> + }
> +
> + @Override
> + protected ProfitBricksApi create(Properties props, Iterable<Module> modules) {
> +
> + Injector injector = newBuilder().modules(modules).overrides(props).buildInjector();
> +
> + return injector.getInstance(ProfitBricksApi.class);
> + }
You shouldn't have to override this method just to get the api.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/225/files#r47705430