You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Zack Shoylev <no...@github.com> on 2013/10/10 23:02:13 UTC
[jclouds-labs-openstack] JCLOUDS-215 - Group config, Group launch,
Scaling policy functionality (#39)
You can merge this Pull Request by running:
git pull https://github.com/rackerlabs/jclouds-labs-openstack rackspace-autoscale2
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-openstack/pull/39
-- Commit Summary --
* JCLOUDS-215 - Adds Group configuration, Group launch, Scaling policy functionality
-- File Changes --
M rackspace-autoscale-us/src/main/java/org/jclouds/rackspace/autoscale/us/v1/AutoscaleUSProviderMetadata.java (1)
M rackspace-autoscale-us/src/test/java/org/jclouds/rackspace/autoscale/us/v1/features/AutoscaleUSGroupApiLiveTest.java (2)
A rackspace-autoscale-us/src/test/java/org/jclouds/rackspace/autoscale/us/v1/features/AutoscaleUSScalingPolicyApiLiveTest.java (32)
M rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/AutoscaleApi.java (29)
M rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/binders/BindCreateGroupToJson.java (17)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/binders/BindLaunchConfigurationToJson.java (77)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/binders/BindScalingPoliciesToJson.java (79)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/binders/BindScalingPolicyToJson.java (70)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/binders/BindToGroupConfigurationRequestPayload.java (64)
M rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/config/AutoscaleHttpApiModule.java (8)
M rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/domain/GroupState.java (12)
M rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/domain/LoadBalancer.java (2)
M rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/domain/ScalingPolicy.java (10)
M rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/domain/ScalingPolicyResponse.java (2)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/features/ExecutionApi.java (57)
M rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/features/GroupApi.java (61)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/features/PolicyApi.java (136)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/features/WebhookApi.java (57)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/functions/ParseGroupLaunchConfigurationResponse.java (90)
M rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/functions/ParseGroupResponse.java (10)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/functions/ParseScalingPoliciesResponse.java (92)
A rackspace-autoscale/src/main/java/org/jclouds/rackspace/autoscale/v1/functions/ParseScalingPolicyResponse.java (87)
M rackspace-autoscale/src/test/java/org/jclouds/rackspace/autoscale/v1/features/GroupApiExpectTest.java (164)
M rackspace-autoscale/src/test/java/org/jclouds/rackspace/autoscale/v1/features/GroupApiLiveTest.java (87)
A rackspace-autoscale/src/test/java/org/jclouds/rackspace/autoscale/v1/features/ScalingPolicyApiExpectTest.java (265)
A rackspace-autoscale/src/test/java/org/jclouds/rackspace/autoscale/v1/features/ScalingPolicyApiLiveTest.java (338)
A rackspace-autoscale/src/test/resources/autoscale_groups_configuration_get_response.json (12)
A rackspace-autoscale/src/test/resources/autoscale_groups_launch_configuration_get_response.json (33)
A rackspace-autoscale/src/test/resources/autoscale_groups_update_configuration_request.json (10)
A rackspace-autoscale/src/test/resources/autoscale_groups_update_launch_configuration_request.json (36)
A rackspace-autoscale/src/test/resources/autoscale_policy_create_request.json (8)
A rackspace-autoscale/src/test/resources/autoscale_policy_create_response.json (18)
A rackspace-autoscale/src/test/resources/autoscale_policy_get_response.json (15)
A rackspace-autoscale/src/test/resources/autoscale_policy_list_response.json (30)
A rackspace-autoscale/src/test/resources/autoscale_policy_update_request.json (6)
A rackspace-autoscale/src/test/resources/autoscale_webhook_get_response.json (17)
A rackspace-autoscale/src/test/resources/autoscale_webhook_update_request.json (6)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-openstack/pull/39.patch
https://github.com/jclouds/jclouds-labs-openstack/pull/39.diff
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> Immutable*.Builders instead of maps: more specifically?
```
ImmutableMap.Builder<X, Y> myData = ImmutableMap.builder();
myData.put("foo", "bar");
...
return myData.build();
```
vs.
```
Map<X, Y> myData = newHashMap();
myData.put("foo", "bar");
...
return myData;
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27275609
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Zack Shoylev <no...@github.com>.
> Map<String,Object> scalingPolicyMap = Maps.newHashMap();
> scalingPoliciesList.add(scalingPolicyMap);
> scalingPolicyMap.put("cooldown", scalingPolicy.getCooldown());
> scalingPolicyMap.put("type", scalingPolicy.getType().toString());
> scalingPolicyMap.put("name", scalingPolicy.getName());
> // A couple of different scaling policies are supported, such as percent or number based, or targeting specific numbers of instances
> - scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), scalingPolicy.getTarget());
> + String targetString = scalingPolicy.getTarget();
> + Integer targetInt = Ints.tryParse(targetString);
> + Float targetFloat;
> + if (targetInt != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetInt);
> + } else if ((targetFloat = Floats.tryParse(targetString)) != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetFloat);
> + } else {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetString);
> + }
I am on the fence for this change. I do like the simple enum type, and the parsing logic is very similar.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7294100
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> older versions used to deserialize to the minimum needed type, i.e. 1 will get deserialized as 1 instead of 1.0; now 1 and 1.0 will be deserialized as 1.0
Learned something new again. No way to configure GSON to revert to the old behaviour?
> When the service becomes stable, it is quite possible much of the parsing logic can be eliminated (especially if the JSON gets restructured).
Fair point. Could we add a code comment then to document that the parsing logic should be revisited if and when the JSON representation settles down?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27344222
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #86](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/86/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27529113
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Zack Shoylev <no...@github.com>.
> Map<String,Object> scalingPolicyMap = Maps.newHashMap();
> scalingPoliciesList.add(scalingPolicyMap);
> scalingPolicyMap.put("cooldown", scalingPolicy.getCooldown());
> scalingPolicyMap.put("type", scalingPolicy.getType().toString());
> scalingPolicyMap.put("name", scalingPolicy.getName());
> // A couple of different scaling policies are supported, such as percent or number based, or targeting specific numbers of instances
> - scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), scalingPolicy.getTarget());
> + String targetString = scalingPolicy.getTarget();
> + Integer targetInt = Ints.tryParse(targetString);
> + Float targetFloat;
> + if (targetInt != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetInt);
> + } else if ((targetFloat = Floats.tryParse(targetString)) != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetFloat);
> + } else {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetString);
> + }
I will go with the code comment for now. Also, live testing here is quite important: it is possible the service becomes more permissive in addition to restructuring.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7316957
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> @@ -40,13 +40,13 @@
> private final String name;
> private final ScalingPolicyType type;
> private final int cooldown;
> - private final int target;
> + private final String target;
Any reason why this is a String and not e.g an Object which could then simply hold the correct type (Int, Float, String) and avoid the parsing logic we are now adding everywhere?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234847
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> + Map<String,Object> scalingPolicyMap = Maps.newHashMap();
> + scalingPoliciesList.add(scalingPolicyMap);
> + scalingPolicyMap.put("cooldown", scalingPolicy.getCooldown());
> + scalingPolicyMap.put("type", scalingPolicy.getType().toString());
> + scalingPolicyMap.put("name", scalingPolicy.getName());
> + // A couple of different scaling policies are supported, such as percent or number based, or targeting specific numbers of instances
> + String targetString = scalingPolicy.getTarget();
> + Integer targetInt = Ints.tryParse(targetString);
> + Float targetFloat;
> + if (targetInt != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetInt);
> + } else if ((targetFloat = Floats.tryParse(targetString)) != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetFloat);
> + } else {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetString);
> + }
See comment above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234832
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
Couple of general comments that apply to many parts of the code:
* I think we prefer Immutable*.Builders to Maps.newHashMap(), Lists.newArrayList() etc., unless we know there are `null` values
* There are some exception messages in `bindToRequest` that may be correct, but may also be copy-paste artifacts
* The "target value as string but apply clever parsing logic" everywhere seems unnecessarily duplicated - can we put that in one of the domain objects rather than sprinkling it all over our parsing?
* We seem to be doing quite a lot of manual JSON maps-to-domain object parsing ourselves here - is there a way to get our web frameworks to do more of this for us?
* Remove APIs from this PR that are actually only placeholders and submit them as a separate PR once they're actually in progress?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27154534
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> ScalingPolicyResponse scalingPolicyResponse =
> new ScalingPolicyResponse(
> (String)scalingPolicyMap.get("name"),
> ScalingPolicyType.getByValue((String)scalingPolicyMap.get("type")).get(),
> ((Double)scalingPolicyMap.get("cooldown")).intValue(),
> - ((Double)scalingPolicyMap.get(targetType.toString())).intValue(),
> - targetType,
> - ImmutableList.copyOf(links),
> - (String) scalingPolicyMap.get("id")
> + Math.floor(d) == d ? Integer.toString(d.intValue()) : Double.toString(d),
`equals` instead of ==?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234862
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #71](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/71/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-26092111
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> Map<String,Object> scalingPolicyMap = Maps.newHashMap();
> scalingPoliciesList.add(scalingPolicyMap);
> scalingPolicyMap.put("cooldown", scalingPolicy.getCooldown());
> scalingPolicyMap.put("type", scalingPolicy.getType().toString());
> scalingPolicyMap.put("name", scalingPolicy.getName());
> // A couple of different scaling policies are supported, such as percent or number based, or targeting specific numbers of instances
> - scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), scalingPolicy.getTarget());
> + String targetString = scalingPolicy.getTarget();
> + Integer targetInt = Ints.tryParse(targetString);
> + Float targetFloat;
> + if (targetInt != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetInt);
> + } else if ((targetFloat = Floats.tryParse(targetString)) != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetFloat);
> + } else {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetString);
> + }
> I do like the simple enum type
Would this make the enum dramatically more complicated? But if, as you say, this parsing thing is still "work in progress", then we can indeed wait until that's settled down before making decisions.
In that case, add a code comment here to document this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7294919
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Zack Shoylev <no...@github.com>.
> GSON
Not aware of a way to revert that behavior.
>> code comment
That is a good point. I will add that to explain about the parsing logic.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27344714
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Zack Shoylev <no...@github.com>.
I refactored the requests (because I got annoyed) but not the responses, for now. I might have to do some more drying for the next phase, and then one last round when the service gets close to release.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27528839
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> + @Path("/groups/{groupId}")
> + PolicyApi getPolicyApiForGroupInZone(@PathParam("groupId") String groupId,
> + @EndpointParam(parser = ZoneToEndpoint.class) @Nullable String zone);
> +
> + /**
> + * Provides access to the executions API. It can be used to execute anonymous webhooks.
> + */
> + @Delegate
> + ExecutionApi getExecutionApiForZone(@EndpointParam(parser = ZoneToEndpoint.class) @Nullable String zone);
> +
> + /**
> + * Provides access to webhook management features.
> + */
> + @Delegate
> + @Path("/groups/{groupId}/policies/{policyId}")
> + WebhookApi getWebhookApiForGroupAndPolicyInZone(@PathParam("groupId") String groupId,
Again, this interface is actually just a placeholder, as far as I can see. Remove from this PR?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234855
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> @@ -54,6 +60,29 @@
> GroupApi getGroupApiForZone(@EndpointParam(parser = ZoneToEndpoint.class) @Nullable String zone);
>
> /**
> + * Provides access to all policy features for scaling Groups.
> + */
> + @Delegate
> + @Path("/groups/{groupId}")
> + PolicyApi getPolicyApiForGroupInZone(@PathParam("groupId") String groupId,
> + @EndpointParam(parser = ZoneToEndpoint.class) @Nullable String zone);
> +
> + /**
> + * Provides access to the executions API. It can be used to execute anonymous webhooks.
> + */
> + @Delegate
> + ExecutionApi getExecutionApiForZone(@EndpointParam(parser = ZoneToEndpoint.class) @Nullable String zone);
Since this is not actually being implemented, remove it from this PR?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234851
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> Map<String,Object> scalingPolicyMap = Maps.newHashMap();
> scalingPoliciesList.add(scalingPolicyMap);
> scalingPolicyMap.put("cooldown", scalingPolicy.getCooldown());
> scalingPolicyMap.put("type", scalingPolicy.getType().toString());
> scalingPolicyMap.put("name", scalingPolicy.getName());
> // A couple of different scaling policies are supported, such as percent or number based, or targeting specific numbers of instances
> - scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), scalingPolicy.getTarget());
> + String targetString = scalingPolicy.getTarget();
> + Integer targetInt = Ints.tryParse(targetString);
> + Float targetFloat;
> + if (targetInt != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetInt);
> + } else if ((targetFloat = Floats.tryParse(targetString)) != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetFloat);
> + } else {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetString);
> + }
Hm...is there any way we could use polymorphism here? Each `TargetType` should _know_ how to correctly return its scaling value, no? Could we move the parsing logic in there and then reduce this to something like:
```
scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), scalingPolicy.getTargetType().toValue(scalingPolicy.getTarget()));
```
or, indeed, have that on the scalingPolicy itself (since it should be able to access both the target type and value):
```
scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), scalingPolicy.getTargetValue() // does the appropriate conversion);
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234823
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> @@ -163,7 +165,8 @@ public void testGetState() {
> GroupApi groupApi = api.getGroupApiForZone(zone);
> String groupId = created.get(zone).get(0).getId();
> GroupState testGroup = groupApi.getState(groupId);
> - assertEquals(testGroup.getId(), groupId);
> + assertNull(testGroup.getId()); // The id recently changed to not be included when getting state.
> + // assertEquals(testGroup.getId(), groupId);
Remove commented-out test?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7361086
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> + Link link = Link.builder().href(URI.create(linkMap.get("href"))).relation(Relation.fromValue(linkMap.get("rel"))).build();
> + links.add(link);
> + }
> +
> + Double d = (Double)scalingPolicyMap.get(targetType.toString()); // GSON only knows double now
> + ScalingPolicyResponse scalingPolicyResponse =
> + new ScalingPolicyResponse(
> + (String)scalingPolicyMap.get("name"),
> + ScalingPolicyType.getByValue((String)scalingPolicyMap.get("type")).get(),
> + ((Double)scalingPolicyMap.get("cooldown")).intValue(),
> + Math.floor(d) == d ? Integer.toString(d.intValue()) : Double.toString(d),
> + targetType,
> + ImmutableList.copyOf(links),
> + (String) scalingPolicyMap.get("id")
> + );
> + scalingPoliciesList.add(scalingPolicyResponse);
Ouch! Is there no way we can get JSON to do some of this parsing for us, i.e. to go to the domain representation more directly?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234875
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Zack Shoylev <no...@github.com>.
> Out of curiosity: what is the GSON limitation here?
GSON always deserializes numbers as a float number (older versions used to deserialize to the minimum needed type, i.e. 1 will get deserialized as 1 instead of 1.0; now 1 and 1.0 will be deserialized as 1.0).
For the requests, the service will not accept "1.0" or 1.0 in the request where an integer is needed.
> Are we making them artificially simple by hiding the complexity of the type of the field in lots of other places in the code?
I am fairly certain that the answer is no. They are just structured in a somewhat weird way. Because the service is still being worked on, I am under the impression that structure might change in the future (and become more straightforward).
I have decided for now it is better to decouple the parsing logic from the domain classes. When the service becomes stable, it is quite possible much of the parsing logic can be eliminated (especially if the JSON gets restructured). This has already happened when comparing request/responses versus earlier versions of autoscale for some calls.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27342806
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> + */
> +public class BindLaunchConfigurationToJson implements MapBinder {
> +
> + private final BindToJsonPayload jsonBinder;
> +
> + @Inject
> + private BindLaunchConfigurationToJson(BindToJsonPayload jsonBinder) {
> + this.jsonBinder = jsonBinder;
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Map<String, Object> postParams) {
> + LaunchConfiguration launchConfigurationRequest = (LaunchConfiguration) postParams.get("launchConfiguration");
> +
> + Map<String, Object> launchConfigurationMap = Maps.newHashMap();
> + Map<String, Object> args = Maps.newHashMap();
Use `ImmutableMap.Builder`s for both of these, or is that not possible?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234826
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> + Float targetFloat;
> + if (targetInt != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetInt);
> + } else if ((targetFloat = Floats.tryParse(targetString)) != null) {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetFloat);
> + } else {
> + scalingPolicyMap.put(scalingPolicy.getTargetType().toString(), targetString);
> + }
> + }
> +
> + return jsonBinder.bindToRequest(request, scalingPoliciesList);
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object toBind) {
> + throw new IllegalStateException("CreateInstance is a POST operation");
See comment above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234837
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #87](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/87/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27571341
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> + * This parses the scaling policy response and decouples domain objects from the json object returned by the service.
> + * @author Zack Shoylev
> + */
> +public class ParseScalingPolicyResponse implements Function<HttpResponse, ScalingPolicyResponse> {
> +
> + private final ParseJson<Map<String, Object>> json;
> +
> + @Inject
> + ParseScalingPolicyResponse(ParseJson<Map<String, Object>> json) {
> + this.json = checkNotNull(json, "json");
> + }
> +
> + /**
> + * Extracts the user password from the json response
> + */
> + @SuppressWarnings("unchecked")
[minor] Comment not relevant to this code?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7361040
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Zack Shoylev <no...@github.com>.
- Immutable*.Builders instead of maps: more specifically?
- Parsing refactoring / not dry: it definitely needs to be refactored. I might prefer to refactor it after I get the last couple of unsupported service calls done.
- Parsing numbers: For this service it is problematic because of how GSON parses numbers now (or because of how inflexible the service is when accepting/returning numbers).
- Clever parsing logic: The reason there is so much manual mapping is to make sure the domain objects make sense and are simple - regardless of what the JSON actually looks like. User experience will degrade if domain objects are more similar to the JSON (at least so it seems in this case). There are some alternatives that can be done to reduce the parsing logic (perhaps a facade of some sort)? But it mostly moves the problem elsewhere.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27269197
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> + server.put("metadata", launchConfigurationRequest.getServerMetadata());
> + List<Map<String, String>> networks = Lists.newArrayList();
> + server.put("networks", networks);
> + for(String networkId : launchConfigurationRequest.getNetworks()) {
> + Map<String, String> network = Maps.newHashMap();
> + network.put("uuid", networkId);
> + networks.add(network);
> + }
> + server.put("personality", launchConfigurationRequest.getPersonalities());
> +
> + return jsonBinder.bindToRequest(request, launchConfigurationMap);
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object toBind) {
> + throw new IllegalStateException("CreateInstance is a POST operation");
Is this the correct message?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234830
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Zack Shoylev <no...@github.com>.
Will merge when tests pass again.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27571045
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> + .getGroupApiForZone("DFW");
> +
> + LaunchConfiguration lc = LaunchConfiguration.builder()
> + .loadBalancers(ImmutableList.of(LoadBalancer.builder().port(8080).id(9099).build()))
> + .serverName("autoscale_server")
> + .serverImageRef("0d589460-f177-4b0f-81c1-8ab8903ac7d8")
> + .serverFlavorRef("2")
> + .serverDiskConfig("AUTO")
> + .serverMetadata(ImmutableMap.of("build_config","core","meta_key_1","meta_value_1","meta_key_2","meta_value_2"))
> + .networks(ImmutableList.of("11111111-1111-1111-1111-111111111111","00000000-0000-0000-0000-000000000000"))
> + .personalities(ImmutableList.of(Personality.builder().path("/root/.csivh").contents("VGhpcyBpcyBhIHRlc3QgZmlsZS4=").build()))
> + .type(LaunchConfigurationType.LAUNCH_SERVER)
> + .build();
> +
> + boolean result = api.updateLaunchConfiguration("1234567890", lc);
> + assertEquals(result, false);
Here and in above: `assertEquals(result, false);` => `assertFalse(result);`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7361061
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
A couple of minor comments but they shouldn't prevent this from being merged. +1 - looks good to me. Thanks, @zack-shoylev!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27540326
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> @@ -120,15 +120,17 @@ public Group apply(HttpResponse from) {
> links.add(link);
> }
>
> + Double d = (Double)scalingPolicyMap.get(targetType.toString()); // GSON only knows double now
Space after the cast?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234858
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> + private final BindToJsonPayload jsonBinder;
> +
> + @Inject
> + private BindScalingPoliciesToJson(BindToJsonPayload jsonBinder) {
> + this.jsonBinder = jsonBinder;
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Map<String, Object> postParams) {
> + List<ScalingPolicy> scalingPoliciesRequest = (List<ScalingPolicy>) postParams.get("scalingPolicies");
> +
> + List<Map<String, Object>> scalingPoliciesList = Lists.newArrayList();
> +
> + for(ScalingPolicy scalingPolicy : scalingPoliciesRequest) {
> + Map<String,Object> scalingPolicyMap = Maps.newHashMap();
[minor] Space before `Object`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7234835
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds-labs-openstack #579](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/579/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39#issuecomment-27529314
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Zack Shoylev <no...@github.com>.
> + */
> +public class BindLaunchConfigurationToJson implements MapBinder {
> +
> + private final BindToJsonPayload jsonBinder;
> +
> + @Inject
> + private BindLaunchConfigurationToJson(BindToJsonPayload jsonBinder) {
> + this.jsonBinder = jsonBinder;
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Map<String, Object> postParams) {
> + LaunchConfiguration launchConfigurationRequest = (LaunchConfiguration) postParams.get("launchConfiguration");
> +
> + Map<String, Object> launchConfigurationMap = Maps.newHashMap();
> + Map<String, Object> args = Maps.newHashMap();
Nulls are only problematic here with immutable lists, I think. ImmutableMap.Builder.put should not throw NPE. I have to still run some live tests for this change.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7326393
Re: [jclouds-labs-openstack] JCLOUDS-215 - Group config, Group
launch, Scaling policy functionality (#39)
Posted by Andrew Phillips <no...@github.com>.
> this.activeCapacity = activeCapacity;
> this.pendingCapacity = pendingCapacity;
> this.desiredCapacity = desiredCapacity;
> this.paused = paused;
> - this.groupInstances = ImmutableList.copyOf(groupInstances);
> + if(groupInstances == null) {
> + this.groupInstances = ImmutableList.of();
> + } else {
> + this.groupInstances = ImmutableList.copyOf(groupInstances);
> + }
[minor] factory out into a "copyOfOrEmpty" or similar method. If not, add a space in `if(...`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/39/files#r7361013