You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Limor Bortman-Stotland <no...@github.com> on 2015/04/08 10:07:19 UTC
[jclouds-labs-openstack] adding: StackApi, Stack and StackResource
(#188)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs-openstack/pull/188
-- Commit Summary --
* adding: StackApi
-- File Changes --
M openstack-heat/pom.xml (8)
M openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/HeatApi.java (8)
A openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/domain/Stack.java (509)
A openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/domain/StackResource.java (250)
A openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/features/StackApi.java (125)
A openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/options/CreateStackOptions.java (249)
A openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/options/ListStackOptions.java (243)
A openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/options/UpdateStackOptions.java (170)
A openstack-heat/src/test/java/org/jclouds/openstack/heat/v1/features/StackApiLiveTest.java (295)
A openstack-heat/src/test/java/org/jclouds/openstack/heat/v1/features/StackApiMockTest.java (317)
A openstack-heat/src/test/java/org/jclouds/openstack/heat/v1/options/ListStackOptionsTest.java (109)
A openstack-heat/src/test/resources/create_stack.json (11)
A openstack-heat/src/test/resources/resources_metadata.json (6)
A openstack-heat/src/test/resources/simple_stack.json (21)
A openstack-heat/src/test/resources/stack_get_response.json (27)
A openstack-heat/src/test/resources/stack_list_response.json (19)
A openstack-heat/src/test/resources/stack_resources_get_response.json (23)
A openstack-heat/src/test/resources/stack_resources_list_response.json (24)
A openstack-heat/src/test/resources/stack_with_environment_and_files.json (23)
A openstack-heat/src/test/resources/stack_with_parameters.json (25)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-openstack/pull/188.patch
https://github.com/jclouds/jclouds-labs-openstack/pull/188.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
Sorry @nacx I don't see any comment about the use of maps in the builders...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-96402422
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertTrue;
> +
> +/**
> + * Tests annotation parsing of {@code StackApi}
> + */
> +@Test(groups = "unit", testName = "StackApiMockTest")
> +public class StackApiMockTest extends BaseHeatApiMockTest {
> +
> + public static final String TEST_STACK_NAME = "testStack";
> + public static final String TEST_STACK_ID = "testStack";
> + public static final String RESOURCES_TEST_NAME = "testResources";
> + public static final String TEST_STACK_RESOURCE_NAME = "cinder_volume";
> +
> +
> + public void testGetAutoStack() throws Exception {
The failure states need to also be tested. For example, with MockResponse().setResponseCode(404), to make sure the @Fallback annotations are also tested.
For example
https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-neutron/src/test/java/org/jclouds/openstack/neutron/v2/features/NetworkApiMockTest.java#L252
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28278924
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + assertThat(stackId).isNotEmpty();
> +
> + boolean success = retry(new Predicate<String>() {
> + public boolean apply(String stackId) {
> + return stackApi.get(stackId).getStatus() == StackStatus.CREATE_COMPLETE;
> + }
> +
> + }, 60, 1, SECONDS).apply(stackId);
> +
> + if (!success) {
> + Assert.fail("Stack didn't get to status CREATE_COMPLETE in 20m.");
> + }
> +
> + UpdateStack updateOptions = UpdateStack.builder().templateUrl(TEMPLATE_URL).build();
> + assertThat(stackApi.update(stackName, stack.getId(), updateOptions)).isTrue();
> + Stack stackAfterUpdate = stackApi.get(stackName, stack.getId());
I say leave it as is, here. However, it is remotely possible that the status might not change right away.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28277822
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + return builder()
> + .name(name)
> + .template(template)
> + .templateUrl(templateUrl)
> + .parameters(parameters)
> + .disableRollback(disableRollback)
> + .files(files)
> + .environment(environment).build();
> + }
> +
> + public static final class Builder {
> +
> + private String name;
> + private String template;
> + private String templateUrl;
> + private Map<String, Object> parameters = ImmutableMap.of();
Instead of re-assigning the map if someone sets it, you could better use an ImmutableMap.Builder here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28424253
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + .template(template)
> + .templateUrl(templateUrl)
> + .parameters(parameters)
> + .disableRollback(disableRollback)
> + .files(files)
> + .environment(environment).build();
> + }
> +
> + public static final class Builder {
> +
> + private String name;
> + private String template;
> + private String templateUrl;
> + private Map<String, Object> parameters = new ImmutableMap.Builder<String, Object>().build();
> + private boolean disableRollback = true;
> + private Map<String, String> files = new ImmutableMap.Builder<String, String>().build();
When I suggested using a builder, I was suggesting another approach: Initialize the builder variable as just the Builder, and call its `putAll` method when someone calls the builder method with a map. Finally, in the `build()` method, call the map builder build to generate the final map.
IMO this approach is more understandable than the current one (which is the same than the old one that just reassigned an existing map).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28953779
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> +/**
> + * Tests parsing and Guice wiring of StackApi
> + */
> +@Test(groups = "live", testName = "StackApiLiveTest")
> +public class StackApiLiveTest extends BaseHeatApiLiveTest {
> +
> + public static final String TEMPLATE_URL = "http://10.5.5.121/Installs/cPaaS/YAML/simple_stack.yaml";
> + protected String stackName = System.getProperty("user.name").replace('.', '-').toLowerCase();
> +
> + public void testList() {
> + for (String region : api.getConfiguredRegions()) {
> + StackApi stackApi = api.getStackApi(region);
> +
> + List<Stack> stacks = stackApi.list();
> +
> + assertThat(stacks).isNotNull();
I mean, since the api method defines [this fallback](https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#diff-667f635b0c1e376a047737c966c674dfR60), the returned list will never be null, so this test is testing nothing. It would be better to assert that the list is not empty, and if possible, to verify that its content is the expected one.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28672664
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + @GET
> + @SelectJson("stack")
> + @Path("/{id}")
> + @Fallback(NullOnNotFoundOr404.class)
> + Stack get(@PathParam("id") String stackId);
> +
> + @Named("stack:create")
> + @POST
> + @SelectJson("stack")
> + Stack create(@BinderParam(BindToJsonPayload.class) CreateStack options);
> +
> + @Named("stack:delete")
> + @DELETE
> + @Path("/{stack_name}/{stack_id}")
> + @Fallback(NullOnNotFoundOr404.class)
> + @Nullable
A primitive can't be null, change the fallback to `FalseOnNotFoundOr404`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28423839
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + Map<String, Object> metadata = api.getStackResourceMetadata(TEST_STACK_NAME, TEST_STACK_ID, RESOURCES_TEST_NAME);
> +
> + /*
> + * Check request
> + */
> + assertEquals(server.getRequestCount(), 2);
> + assertAuthentication(server);
> + assertRequest(server.takeRequest(), "GET", BASE_URI + "/stacks/" + TEST_STACK_NAME + "/" + TEST_STACK_ID + "/resources/" + RESOURCES_TEST_NAME + "/metadata");
> + assertThat(metadata).isNotEmpty();
> +
> +
> + } finally {
> + server.shutdown();
> + }
> + }
> +}
For each method that defines a fallback, there must exist a test that exercises it. Add all missing tests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28425029
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + assertThat(stackFromList).isNotNull();
> + assertThat(stackFromList.getId()).isEqualTo(stack.getId());
> + for (String parmName : parameters.keySet()) {
> + assertThat(stackFromList.getParameters().containsKey(parmName)).isTrue();
> + }
> +
> + }
> + }
> +
> + public void testCreateWithFilesAndEnvironment() throws IOException, ParseException {
> + for (String region : api.getConfiguredRegions()) {
> + StackApi stackApi = api.getStackApi(region);
> + JSONParser parser = new JSONParser();
> + Map<String, String> files = new HashMap<String, String>();
> + Object obj = parser.parse(stringFromResource("/stack_with_environment_and_files.json"));
> + files.put("LCP-VolumeB.template.yaml", "{\"resources\":{\"ALU-LCP-Block-StorageB\":{\"properties\":{\"description\":\"Used for VM \\/storage partition.\",\"name\":{\"str_replace\":{\"template\":\"$stk-s01c$cardh0\",\"params\":{\"$card\":{\"get_param\":\"cardvB\"},\"$stk\":{\"get_param\":\"deployment_prefix\"}}}},\"size\":{\"get_param\":\"storage_size\"}},\"type\":\"OS::Cinder::Volume\"}},\"heat_template_version\":\"2013-05-23\",\"description\":\"Template to set up volumes for the initial deployment\",\"parameters\":{\"cardvB\":{\"default\":\"00\",\"description\":\"The (2 digit) virtual card number for server attached to volume when on shelf 0\",\"constraints\":[{\"length\":{\"min\":\"2\",\"max\":\"2\"}},{\"allowed_pattern\":\"^[0-9][0-9]$\"}],\"type\":\"string\"},\"deployment_prefix\":{\"description\":\"Name prefix for resources. It should match the system prefix\\nused in the FI worksheet but this is not enforced.\",\"constraints\":[{\"length\":{\"min\":\"5\",\"max\":\"1
4\"}},{\"allowed_pattern\":\"^[a-zA-Z0-9][a-zA-Z0-9_-]*$\"}],\"type\":\"string\"},\"storage_size\":{\"default\":\"4\",\"description\":\"GBs for block storage\",\"constraints\":[{\"range\":{\"min\":\"4\"}}],\"type\":\"number\"},\"group_index\":{\"default\":[],\"description\":\"List of strings each pointing to one of the other parameters that is steped up for each additional \\nresource of the same type added to the stack (e.g. at growth event)\",\"type\":\"comma_delimited_list\"}},\"outputs\":{\"volumeId\":{\"description\":\"VolumeId\",\"value\":{\"get_resource\":\"ALU-LCP-Block-StorageB\"}}}}");
Can this be a stringFromResource, also?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28102314
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + return this;
> + }
> +
> + /**
> + * @param environment - used to affect the runtime behaviour of the template
> + */
> + public Builder environment(String environment) {
> + this.environment = environment;
> + return this;
> + }
> +
> + public CreateStack build() {
> + CreateStack result = new AutoValue_CreateStack(
> + this.name,
> + this.template != null ? template : null,
> + this.templateUrl != null ? templateUrl : null,
These null checks do nothing. Just assign the field.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28424340
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> <scope>provided</scope>
> </dependency>
> <dependency>
> <groupId>com.google.auto.service</groupId>
> <artifactId>auto-service</artifactId>
> + <version>1.0-rc2</version>
http://search.maven.org/#artifactdetails%7Ccom.google.auto.value%7Cauto-value%7C1.0%7Cjar
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28170456
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + boolean delete(@PathParam("stack_name") String name, @PathParam("stack_id") String id);
> +
> + @Named("stack:update")
> + @PUT
> + @Path("/{stack_name}/{stack_id}")
> + @Fallback(NullOnNotFoundOr404.class)
> + @Nullable
> + boolean update(@PathParam("stack_name") String name, @PathParam("stack_id") String id, @BinderParam(BindToJsonPayload.class) UpdateStack options);
> +
> +
> + @Named("stack:list_resources")
> + @GET
> + @SelectJson("resources")
> + @Path("/{stack_name}/{stack_id}/resources")
> + @Fallback(EmptyFluentIterableOnNotFoundOr404.class)
> + FluentIterable<StackResource> listStackResources(@PathParam("stack_name") String stackName, @PathParam("stack_id") String stackId);
Will List<StackResource> work here instead of the Guava collection? We are trying to depend less on those in user-facing code.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28277931
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
> + return this;
> + }
> +
> + public CreateStack build() {
> + String missing = "";
> +
> + if (name == null) {
> + missing += " name";
> + }
> +
> +
> + if (template == null && templateUrl == null) {
> + missing += " template and templateUrl";
> + }
> +
> + if (!missing.isEmpty()) {
question... when we send to heat Create Stack/UpdateStack we have to send template or templateUrl , So do we want an early validation here or do we want heat to fail?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28302573
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
> + return new AutoValue_CreateStack.Builder().disableRollback(true).files(null).environment(null);
> + }
> +
> + public Builder toBuilder() {
> + return builder()
> + .name(getName())
> + .template(getTemplate())
> + .templateUrl(getTemplateUrl())
> + .parameters(getParameters())
> + .disableRollback(isDisableRollback())
> + .files(getFiles())
> + .environment(getEnvironment());
> + }
> +
> + @SerializedNames({"stack_name", "template", "template_url", "parameters", "disable_rollback", "files", "environment" })
> + private static CreateStack create(String name, @Nullable String template, @Nullable String templateUrl, @Nullable Map<String, Object> parameters, boolean disableRollback, @Nullable Map<String, String> files, @Nullable String environment) {
It shouldn't be. I removed the nullable
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28665295
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
this is the auto-value, I fixed it.. i am talking about the auto-service
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-92019311
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
> +
> + /**
> + * @return Specifies the self-navigating JSON document paths.
> + */
> + public abstract Set<Link> getLinks();
> +
> + @SerializedNames({"resource_name", "logical_resource_id", "resource_status_reason", "updated_time", "required_by", "resource_status", "physical_resource_id", "resource_type", "links"})
> + private static StackResource create(String name, String logicalResourceId, String statusReason, Date updated, Set<String> requiredBy,
> + StackResourceStatus status, String physicalResourceId, String resourceType, Set<Link> links) {
> + return new AutoValue_StackResource(
> + name,
> + logicalResourceId,
> + physicalResourceId,
> + status,
> + statusReason,
> + requiredBy != null ? ImmutableSet.copyOf(requiredBy) : null,
Add nullable to the signature and to the get
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28665336
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
I really like how this is turning out. Great work @limorbortman!
My one regret is that the autovalue builders are not officially out yet. But this will still be very easy to refactor once they are.
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-92499134
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + * @param environment - used to affect the runtime behaviour of the template
> + */
> + public Builder environment(String environment) {
> + this.environment = environment;
> + return this;
> + }
> +
> + public CreateStack build() {
> + CreateStack result = new AutoValue_CreateStack(
> + this.name,
> + this.template != null ? template : null,
> + this.templateUrl != null ? templateUrl : null,
> + this.parameters != null ? ImmutableMap.copyOf(this.parameters) : null,
> + this.disableRollback,
> + this.files != null ? ImmutableMap.copyOf(this.files) : null,
> + this.environment != null ? environment : null );
Same here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28424354
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + /**
> + * @return Specifies the self-navigating JSON document paths.
> + */
> + public abstract Set<Link> getLinks();
> +
> + @SerializedNames({"resource_name", "logical_resource_id", "resource_status_reason", "updated_time", "required_by", "resource_status", "physical_resource_id", "resource_type", "links"})
> + private static StackResource create(String name, String logicalResourceId, String statusReason, Date updated, Set<String> requiredBy,
> + StackResourceStatus status, String physicalResourceId, String resourceType, Set<Link> links) {
> + return new AutoValue_StackResource(
> + name,
> + logicalResourceId,
> + physicalResourceId,
> + status,
> + statusReason,
> + requiredBy != null ? ImmutableSet.copyOf(requiredBy) : null,
Field is not marked as nullable. Remove the null check or annotate the field accordingly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28423699
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + @Named("stack:create")
> + @POST
> + @SelectJson("stack")
> + Stack create(@BinderParam(BindToJsonPayload.class) CreateStack options);
> +
> + @Named("stack:delete")
> + @DELETE
> + @Path("/{stack_name}/{stack_id}")
> + @Fallback(NullOnNotFoundOr404.class)
> + @Nullable
> + boolean delete(@PathParam("stack_name") String name, @PathParam("stack_id") String id);
> +
> + @Named("stack:update")
> + @PUT
> + @Path("/{stack_name}/{stack_id}")
> + @Fallback(NullOnNotFoundOr404.class)
Write operations as PUT/POST shouldn't define fallbacks for 404 responses. Remove it (see [JCLOUDS-691](https://issues.apache.org/jira/browse/JCLOUDS-691)).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28424070
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
> + String stackName = getName();
> + CreateStack createStack = CreateStack.builder().name(stackName).templateUrl(TEMPLATE_URL).build();
> + Stack stack = stackApi.create(createStack);
> + assertThat(stack).isNotNull();
> + assertThat(stack.getId()).isNotEmpty();
> + assertThat(stackApi.delete(stackName, stack.getId())).isTrue();
> + Stack stackAfterDelete = stackApi.get(stackName, stack.getId());
> + assertThat(stackAfterDelete).isNotNull();
> + }
> + }
> +
> + public void testCreateWithDisableRollback() {
> + for (String region : api.getConfiguredRegions()) {
> + StackApi stackApi = api.getStackApi(region);
> + CreateStack createStack = CreateStack.builder().name(getName()).templateUrl(TEMPLATE_URL).disableRollback(false).build();
> + Stack stack = stackApi.create(createStack);
add cleanup()
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28665374
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> @@ -98,13 +98,21 @@
> <dependency>
> <groupId>com.google.auto.value</groupId>
> <artifactId>auto-value</artifactId>
> + <version>1.0-rc2</version>
The latest version is 1.0
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28096847
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + return physicalResourceId;
> + }
> +
> + public Set<String> getRequiredBy() {
> + return requiredBy;
> + }
> +
> + public String getResourceType() {
> + return resourceType;
> + }
> +
> + public Date getUpdated() {
> + return updated;
> + }
> +
> + public enum Status {
Let's keep enums in their own separate files. Makes it easier to read.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28097985
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + /*
> + * Check request
> + */
> + assertThat(server.getRequestCount()).isEqualTo(2);
> + assertAuthentication(server);
> + assertRequest(server.takeRequest(), "GET", BASE_URI + "/stacks");
> +
> + /*
> + * Check response
> + */
> + assertThat(stacks).isEmpty();
> + } finally {
> + server.shutdown();
> + }
> + }
Add also the tests for the list methods that receive an options parameter.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28425081
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + return this;
> + }
> +
> + public CreateStack build() {
> + String missing = "";
> +
> + if (name == null) {
> + missing += " name";
> + }
> +
> +
> + if (template == null && templateUrl == null) {
> + missing += " template and templateUrl";
> + }
> +
> + if (!missing.isEmpty()) {
In general, we should probably be moving away from client-side validation where possible.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28278130
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
@limorbortman : Looks good. I will run some tests and merge today. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-97502203
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + .template(template)
> + .templateUrl(templateUrl)
> + .parameters(parameters)
> + .disableRollback(disableRollback)
> + .files(files)
> + .environment(environment).build();
> + }
> +
> + public static final class Builder {
> +
> + private String name;
> + private String template;
> + private String templateUrl;
> + private Map<String, Object> parameters = new ImmutableMap.Builder<String, Object>().build();
> + private boolean disableRollback = true;
> + private Map<String, String> files = new ImmutableMap.Builder<String, String>().build();
Yep, leave fields uninitialized, the rest of the code looks good.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28998504
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
Alright. I think it's ready to merge. @nacx ?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-94950349
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + @Nullable public abstract Date getCreated();
> +
> + /**
> + * @return the date this Stack was last updated.
> + */
> + @Nullable public abstract Date getUpdated();
> +
> + /**
> + * @return the timeout of this Stack (in minutes).
> + */
> + @Nullable public abstract int getTimeoutMins();
> +
> + /**
> + * @return true is disableRollback is true
> + */
> + @Nullable public abstract boolean isDisableRollback();
Same here. Primitives can't be null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28423625
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
> +/**
> + * Tests parsing and Guice wiring of StackApi
> + */
> +@Test(groups = "live", testName = "StackApiLiveTest")
> +public class StackApiLiveTest extends BaseHeatApiLiveTest {
> +
> + public static final String TEMPLATE_URL = "http://10.5.5.121/Installs/cPaaS/YAML/simple_stack.yaml";
> + protected String stackName = System.getProperty("user.name").replace('.', '-').toLowerCase();
> +
> + public void testList() {
> + for (String region : api.getConfiguredRegions()) {
> + StackApi stackApi = api.getStackApi(region);
> +
> + List<Stack> stacks = stackApi.list();
> +
> + assertThat(stacks).isNotNull();
Check what I add. Is this enough?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28665362
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
HI @zack-shoylev and @nacx ,
Can you check?
Thanks
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-94660815
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> <scope>provided</scope>
> </dependency>
> <dependency>
> <groupId>com.google.auto.service</groupId>
> <artifactId>auto-service</artifactId>
> + <version>1.0-rc2</version>
1.0, I think, don't use the rc.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28096880
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + this.templateUrl = templateUrl;
> + return this;
> + }
> +
> + /**
> + * @see UpdateStack#getParameters()
> + */
> + public Builder parameters(Map<String, Object> parameters) {
> + this.parameters = parameters;
> + return this;
> + }
> +
> + public UpdateStack build() {
> + UpdateStack result = new AutoValue_UpdateStack(
> + this.template != null ? template : null,
> + this.templateUrl != null ? templateUrl : null,
Remove the redundant null checks.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28424471
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + public String value() {
> + return CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_HYPHEN, name());
> + }
> +
> + @Override
> + public String toString() {
> + return value();
> + }
> +
> + /**
> + * This provides GSON enum support in jclouds.
> + * @param name The string representation of this enum value.
> + * @return The corresponding enum value.
> + */
> +
> + public static Status fromValue(String status) {
Is this going to handle a null or missing value as UNRECOGNIZED?
In any case, shouldn't really be an issue.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28097739
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> @@ -0,0 +1,11 @@
> +{
> + "stack": {
> + "id": "3095aefc-09fb-4bc7-b1f0-f21a304e864c",
> + "links": [
> + {
> + "href": "http://192.168.123.200:8004/v1/eb1c63a4f77141548385f113a28f0f52/stacks/simple_stack/3095aefc-09fb-4bc7-b1f0-f21a304e864c",
> + "rel": "self"
> + }
> + ]
> + }
> +}
Small nit, but terminate json with newlines.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28102433
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + @Nullable public abstract String getSatusReason();
> +
> + /**
> + * @return the date this Stack was created.
> + */
> + @Nullable public abstract Date getCreated();
> +
> + /**
> + * @return the date this Stack was last updated.
> + */
> + @Nullable public abstract Date getUpdated();
> +
> + /**
> + * @return the timeout of this Stack (in minutes).
> + */
> + @Nullable public abstract int getTimeoutMins();
With increased usage of auto, we might want a checkstyle for that :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28523376
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + @Nullable public abstract String getSatusReason();
> +
> + /**
> + * @return the date this Stack was created.
> + */
> + @Nullable public abstract Date getCreated();
> +
> + /**
> + * @return the date this Stack was last updated.
> + */
> + @Nullable public abstract Date getUpdated();
> +
> + /**
> + * @return the timeout of this Stack (in minutes).
> + */
> + @Nullable public abstract int getTimeoutMins();
Primitives can't be null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28423607
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + boolean delete(@PathParam("stack_name") String name, @PathParam("stack_id") String id);
> +
> + @Named("stack:update")
> + @PUT
> + @Path("/{stack_name}/{stack_id}")
> + @Fallback(NullOnNotFoundOr404.class)
> + @Nullable
> + boolean update(@PathParam("stack_name") String name, @PathParam("stack_id") String id, @BinderParam(BindToJsonPayload.class) UpdateStack options);
> +
> +
> + @Named("stack:list_resources")
> + @GET
> + @SelectJson("resources")
> + @Path("/{stack_name}/{stack_id}/resources")
> + @Fallback(EmptyFluentIterableOnNotFoundOr404.class)
> + FluentIterable<StackResource> listStackResources(@PathParam("stack_name") String stackName, @PathParam("stack_id") String stackId);
This of course applies to the rest of the code that uses FluentIterable as well :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28278035
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
HI @zack-shoylev and @nacx
can you please recheck?
Thanks
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-97317678
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> <scope>provided</scope>
> </dependency>
> <dependency>
> <groupId>com.google.auto.service</groupId>
> <artifactId>auto-service</artifactId>
> + <version>1.0-rc2</version>
Good point.
In any case, the right way to use this dependency, if I remember correctly, would be this:
<dependency>
<groupId>com.google.auto.service</groupId>
<artifactId>auto-service</artifactId>
<scope>provided</scope>
</dependency>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28258567
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> +import org.jclouds.openstack.v2_0.domain.Link;
> +import org.jclouds.openstack.v2_0.domain.Resource;
> +
> +import javax.inject.Named;
> +import java.beans.ConstructorProperties;
> +import java.util.Date;
> +import java.util.List;
> +import java.util.Map;
> +import java.util.Set;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * Representation of an OpenStack Heat Stack.
> + */
> +public class Stack extends Resource {
Avoid extending types (because of autovalue, will just require more refactoring later).
Use autovalue, similar to, for example:
https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-poppy/src/main/java/org/jclouds/openstack/poppy/v1/domain/CreateService.java
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28097907
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
> <scope>provided</scope>
> </dependency>
> <dependency>
> <groupId>com.google.auto.service</groupId>
> <artifactId>auto-service</artifactId>
> + <version>1.0-rc2</version>
this is the auto-value, I fixed it.. i am talking about the auto-service
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28201837
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + @MapBinder(CreateStackOptions.class)
> + Stack create(@PayloadParam("stack_name") String name, CreateStackOptions... options);
> +
> + @Named("stack:delete")
> + @DELETE
> + @Path("/{stack_name}/{stack_id}")
> + @Fallback(NullOnNotFoundOr404.class)
> + @Nullable
> + boolean delete(@PathParam("stack_name") String name, @PathParam("stack_id") String id);
> +
> + @Named("stack:update")
> + @PUT
> + @Path("/{stack_name}/{stack_id}")
> + @Fallback(NullOnNotFoundOr404.class)
> + @Nullable
> + boolean update(@PathParam("stack_name") String name, @PathParam("stack_id") String id, UpdateStackOptions... options);
Avoid using MapBinders if possible and unless it greatly enhanced user experience.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28098201
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + <groupId>com.google.auto.service</groupId>
> + <artifactId>auto-service</artifactId>
> + <scope>provided</scope>
> + </dependency>
> + <dependency>
> + <groupId>com.google.auto.value</groupId>
> + <artifactId>auto-value</artifactId>
> + <version>1.0</version>
> + <scope>provided</scope>
> + </dependency>
> + <dependency>
> + <groupId>com.googlecode.json-simple</groupId>
> + <artifactId>json-simple</artifactId>
> + <version>1.1</version>
> + <scope>test</scope>
> + </dependency>
> </dependencies>
Reindent back to 2 spaces
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28423532
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + return new AutoValue_CreateStack.Builder().disableRollback(true).files(null).environment(null);
> + }
> +
> + public Builder toBuilder() {
> + return builder()
> + .name(getName())
> + .template(getTemplate())
> + .templateUrl(getTemplateUrl())
> + .parameters(getParameters())
> + .disableRollback(isDisableRollback())
> + .files(getFiles())
> + .environment(getEnvironment());
> + }
> +
> + @SerializedNames({"stack_name", "template", "template_url", "parameters", "disable_rollback", "files", "environment" })
> + private static CreateStack create(String name, @Nullable String template, @Nullable String templateUrl, @Nullable Map<String, Object> parameters, boolean disableRollback, @Nullable Map<String, String> files, @Nullable String environment) {
The name is marked as nullable in the field declaration but not marked as nullable here. Is it nullable or not?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28424175
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
Nice job @limorbortman. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-97587544
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> +/**
> + * Tests parsing and Guice wiring of StackApi
> + */
> +@Test(groups = "live", testName = "StackApiLiveTest")
> +public class StackApiLiveTest extends BaseHeatApiLiveTest {
> +
> + public static final String TEMPLATE_URL = "http://10.5.5.121/Installs/cPaaS/YAML/simple_stack.yaml";
> + protected String stackName = System.getProperty("user.name").replace('.', '-').toLowerCase();
> +
> + public void testList() {
> + for (String region : api.getConfiguredRegions()) {
> + StackApi stackApi = api.getStackApi(region);
> +
> + List<Stack> stacks = stackApi.list();
> +
> + assertThat(stacks).isNotNull();
Is this enough to guarantee that the call works? Shouldn't we better check if it is empty, or setup the test in a way that it returns a non-empty list, so we can assert that everything (including deserialization) works?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28424589
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + return this;
> + }
> +
> + public CreateStack build() {
> + String missing = "";
> +
> + if (name == null) {
> + missing += " name";
> + }
> +
> +
> + if (template == null && templateUrl == null) {
> + missing += " template and templateUrl";
> + }
> +
> + if (!missing.isEmpty()) {
@limorbortman Does heat fail properly with a decent descriptive fail message? If so, I don't think we need this. It's basically a type of code/feature duplication and the failing forward benefit does not seem that great to me in this case.
However, if the heat validation message is something cryptic, I say keep this code.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28357181
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + .template(template)
> + .templateUrl(templateUrl)
> + .parameters(parameters)
> + .disableRollback(disableRollback)
> + .files(files)
> + .environment(environment).build();
> + }
> +
> + public static final class Builder {
> +
> + private String name;
> + private String template;
> + private String templateUrl;
> + private Map<String, Object> parameters = new ImmutableMap.Builder<String, Object>().build();
> + private boolean disableRollback = true;
> + private Map<String, String> files = new ImmutableMap.Builder<String, String>().build();
I suggested that because IIRC the fields in the builder were actually initialised to an empty map. What you say also makes sense. In that case, let's leave the fields uninitialised and don't use builders, as it does not make sense.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28997578
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + .template(template)
> + .templateUrl(templateUrl)
> + .parameters(parameters)
> + .disableRollback(disableRollback)
> + .files(files)
> + .environment(environment).build();
> + }
> +
> + public static final class Builder {
> +
> + private String name;
> + private String template;
> + private String templateUrl;
> + private Map<String, Object> parameters = ImmutableMap.of();
> + private boolean disableRollback = true;
> + private Map<String, String> files = ImmutableMap.of();
Same here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28424263
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + /*
> + * Check request
> + */
> + assertEquals(server.getRequestCount(), 2);
> + assertAuthentication(server);
> + assertRequest(server.takeRequest(), "GET", BASE_URI + "/stacks/" + TEST_STACK_NAME + "/" + TEST_STACK_ID + "/resources/" + RESOURCES_TEST_NAME + "/metadata");
> + assertThat(metadata).isNotEmpty();
> +
> +
> + } finally {
> + server.shutdown();
> + }
> + }
> +}
> +
> +
extra newlines?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28102489
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
> + String stackName = getName();
> + CreateStack createStack = CreateStack.builder().name(stackName).templateUrl(TEMPLATE_URL).build();
> + Stack stack = stackApi.create(createStack);
> + assertThat(stack).isNotNull();
> + assertThat(stack.getId()).isNotEmpty();
> + assertThat(stackApi.delete(stackName, stack.getId())).isTrue();
> + Stack stackAfterDelete = stackApi.get(stackName, stack.getId());
> + assertThat(stackAfterDelete).isNotNull();
> + }
> + }
> +
> + public void testCreateWithDisableRollback() {
> + for (String region : api.getConfiguredRegions()) {
> + StackApi stackApi = api.getStackApi(region);
> + CreateStack createStack = CreateStack.builder().name(getName()).templateUrl(TEMPLATE_URL).disableRollback(false).build();
> + Stack stack = stackApi.create(createStack);
Several tests create stacks but don't take care of deleting them. Add an `afterClass` method or a proper clenaup in each test to avoid leaking resources after running the live tests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28424810
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Limor Bortman-Stotland <no...@github.com>.
> <scope>provided</scope>
> </dependency>
> <dependency>
> <groupId>com.google.auto.service</groupId>
> <artifactId>auto-service</artifactId>
> + <version>1.0-rc2</version>
I looked for it all i found is : http://mvnrepository.com/artifact/com.google.auto.service/auto-service/1.0-rc2
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28127749
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Ignasi Barrera <no...@github.com>.
Just noted a last comment about the use of maps in the builders.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-95544168
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
> + .template(template)
> + .templateUrl(templateUrl)
> + .parameters(parameters)
> + .disableRollback(disableRollback)
> + .files(files)
> + .environment(environment).build();
> + }
> +
> + public static final class Builder {
> +
> + private String name;
> + private String template;
> + private String templateUrl;
> + private Map<String, Object> parameters = new ImmutableMap.Builder<String, Object>().build();
> + private boolean disableRollback = true;
> + private Map<String, String> files = new ImmutableMap.Builder<String, String>().build();
@nacx, I like the approach you are suggesting, but I don't think it's consistent with how we handle collections in other places. We usually completely override the existing collection, instead of adding more elements.
For example, here, parameetrs and files should not be initialized at all (because they are nullable, and will be skipped when json is generated). Just keep them as private Map<String, Object> parameters;
The builder build() already checks for null values and copies the map if needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files#r28991929
Re: [jclouds-labs-openstack] adding: StackApi, Stack and
StackResource (#188)
Posted by Zack Shoylev <no...@github.com>.
@limorbortman the comment is for this line:
https://github.com/limorbortman/jclouds-labs-openstack/blob/adding_heat/openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/options/CreateStack.java#L101
I'm not sure why it's not showing up, but I can see it in
https://github.com/jclouds/jclouds-labs-openstack/pull/188/files
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/188#issuecomment-96814351