You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrea Turli <no...@github.com> on 2014/02/19 17:04:07 UTC
[jclouds] Refactor SoftLayer CCI support (#296)
It modernizes SoftLayer CCI support by re-using the same API calls used by SoftLayer Python client.
The implementation should be not more easily maintainable and robust.
You can merge this Pull Request by running:
git pull https://github.com/andreaturli/jclouds refactor/softlayer
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/296
-- Commit Summary --
* [JCLOUDS-474] refactor SoftLayer support
-- File Changes --
M providers/softlayer/pom.xml (15)
M providers/softlayer/src/main/java/org/jclouds/softlayer/SoftLayerApi.java (28)
M providers/softlayer/src/main/java/org/jclouds/softlayer/SoftLayerApiMetadata.java (30)
M providers/softlayer/src/main/java/org/jclouds/softlayer/SoftLayerProviderMetadata.java (32)
D providers/softlayer/src/main/java/org/jclouds/softlayer/binders/ProductOrderToJson.java (133)
A providers/softlayer/src/main/java/org/jclouds/softlayer/binders/VirtualGuestToJson.java (146)
M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/config/SoftLayerComputeServiceContextModule.java (85)
M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/DatacenterToLocation.java (10)
A providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/OperatingSystemToImage.java (145)
D providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/ProductItemToImage.java (187)
D providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/ProductItems.java (90)
D providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/ProductItemsToHardware.java (124)
A providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/VirtualGuestToHardware.java (73)
A providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/VirtualGuestToImage.java (154)
M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/VirtualGuestToNodeMetadata.java (84)
M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/options/SoftLayerTemplateOptions.java (32)
M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/strategy/SoftLayerComputeServiceAdapter.java (264)
M providers/softlayer/src/main/java/org/jclouds/softlayer/config/SoftLayerParserModule.java (8)
R providers/softlayer/src/main/java/org/jclouds/softlayer/config/SoftLayerRestClientModule.java (10)
M providers/softlayer/src/main/java/org/jclouds/softlayer/domain/Address.java (179)
A providers/softlayer/src/main/java/org/jclouds/softlayer/domain/ContainerVirtualGuestConfiguration.java (286)
A providers/softlayer/src/main/java/org/jclouds/softlayer/domain/ContainerVirtualGuestConfigurationOption.java (117)
M providers/softlayer/src/main/java/org/jclouds/softlayer/domain/Datacenter.java (27)
M providers/softlayer/src/main/java/org/jclouds/softlayer/domain/OperatingSystem.java (83)
M providers/softlayer/src/main/java/org/jclouds/softlayer/domain/Password.java (2)
M providers/softlayer/src/main/java/org/jclouds/softlayer/domain/PowerState.java (2)
M providers/softlayer/src/main/java/org/jclouds/softlayer/domain/ProductItem.java (206)
D providers/softlayer/src/main/java/org/jclouds/softlayer/domain/ProductItemCategory.java (154)
M providers/softlayer/src/main/java/org/jclouds/softlayer/domain/ProductItemPrice.java (204)
D providers/softlayer/src/main/java/org/jclouds/softlayer/domain/ProductOrder.java (224)
D providers/softlayer/src/main/java/org/jclouds/softlayer/domain/ProductOrderReceipt.java (136)
D providers/softlayer/src/main/java/org/jclouds/softlayer/domain/ProductPackage.java (208)
A providers/softlayer/src/main/java/org/jclouds/softlayer/domain/SoftwareDescription.java (308)
A providers/softlayer/src/main/java/org/jclouds/softlayer/domain/SoftwareLicense.java (142)
A providers/softlayer/src/main/java/org/jclouds/softlayer/domain/VirtualDiskImage.java (233)
M providers/softlayer/src/main/java/org/jclouds/softlayer/domain/VirtualGuest.java (157)
A providers/softlayer/src/main/java/org/jclouds/softlayer/domain/VirtualGuestBlockDevice.java (252)
D providers/softlayer/src/main/java/org/jclouds/softlayer/features/AccountApi.java (59)
M providers/softlayer/src/main/java/org/jclouds/softlayer/features/DatacenterApi.java (28)
R providers/softlayer/src/main/java/org/jclouds/softlayer/features/SoftwareDescriptionApi.java (40)
M providers/softlayer/src/main/java/org/jclouds/softlayer/features/VirtualGuestApi.java (143)
M providers/softlayer/src/main/java/org/jclouds/softlayer/handlers/SoftLayerErrorHandler.java (5)
D providers/softlayer/src/main/java/org/jclouds/softlayer/predicates/ProductItemPredicates.java (154)
D providers/softlayer/src/main/java/org/jclouds/softlayer/predicates/ProductPackagePredicates.java (41)
M providers/softlayer/src/main/java/org/jclouds/softlayer/reference/SoftLayerConstants.java (30)
D providers/softlayer/src/test/java/org/jclouds/softlayer/SoftLayerApiTest.java (55)
M providers/softlayer/src/test/java/org/jclouds/softlayer/SoftLayerProviderTest.java (8)
D providers/softlayer/src/test/java/org/jclouds/softlayer/binders/ProductOrderToJsonTest.java (94)
M providers/softlayer/src/test/java/org/jclouds/softlayer/compute/SoftLayerComputeServiceAdapterLiveTest.java (28)
M providers/softlayer/src/test/java/org/jclouds/softlayer/compute/SoftLayerComputeServiceLiveTest.java (19)
A providers/softlayer/src/test/java/org/jclouds/softlayer/compute/SoftLayerExperimentIntegrationTest.java (102)
R providers/softlayer/src/test/java/org/jclouds/softlayer/compute/SoftLayerTemplateBuilderIntegrationTest.java (66)
M providers/softlayer/src/test/java/org/jclouds/softlayer/compute/functions/DatacenterToLocationTest.java (19)
D providers/softlayer/src/test/java/org/jclouds/softlayer/compute/functions/ProductItemToImageTest.java (264)
D providers/softlayer/src/test/java/org/jclouds/softlayer/compute/functions/ProductItemsTest.java (143)
D providers/softlayer/src/test/java/org/jclouds/softlayer/compute/functions/ProductItemsToHardwareTest.java (157)
M providers/softlayer/src/test/java/org/jclouds/softlayer/compute/functions/VirtualGuestToNodeMetadataTest.java (31)
D providers/softlayer/src/test/java/org/jclouds/softlayer/features/AccountApiTest.java (55)
R providers/softlayer/src/test/java/org/jclouds/softlayer/features/BaseSoftLayerApiExpectTest.java (17)
M providers/softlayer/src/test/java/org/jclouds/softlayer/features/BaseSoftLayerApiLiveTest.java (3)
A providers/softlayer/src/test/java/org/jclouds/softlayer/features/DatacenterApiExpectTest.java (95)
M providers/softlayer/src/test/java/org/jclouds/softlayer/features/DatacenterApiLiveTest.java (49)
D providers/softlayer/src/test/java/org/jclouds/softlayer/features/DatacenterApiTest.java (85)
D providers/softlayer/src/test/java/org/jclouds/softlayer/features/ProductPackageApiLiveTest.java (218)
D providers/softlayer/src/test/java/org/jclouds/softlayer/features/ProductPackageApiTest.java (55)
A providers/softlayer/src/test/java/org/jclouds/softlayer/features/SoftwareDescriptionApiExpectTest.java (64)
R providers/softlayer/src/test/java/org/jclouds/softlayer/features/SoftwareDescriptionApiLiveTest.java (33)
M providers/softlayer/src/test/java/org/jclouds/softlayer/features/VirtualGuestApiExpectTest.java (135)
M providers/softlayer/src/test/java/org/jclouds/softlayer/features/VirtualGuestApiLiveTest.java (212)
D providers/softlayer/src/test/java/org/jclouds/softlayer/features/VirtualGuestApiTest.java (175)
R providers/softlayer/src/test/java/org/jclouds/softlayer/internal/BaseSoftLayerParseTest.java (23)
R providers/softlayer/src/test/java/org/jclouds/softlayer/parse/CreateVirtualGuestResponseTest.java (56)
A providers/softlayer/src/test/java/org/jclouds/softlayer/parse/GetAllObjectsResponseTest.java (64)
A providers/softlayer/src/test/java/org/jclouds/softlayer/parse/GetDatacenterResponseTest.java (67)
A providers/softlayer/src/test/java/org/jclouds/softlayer/parse/GetVirtualGuestResponseTest.java (69)
A providers/softlayer/src/test/java/org/jclouds/softlayer/parse/ListDatacentersResponseTest.java (118)
A providers/softlayer/src/test/java/org/jclouds/softlayer/parse/ListVirtualGuestsResponseTest.java (72)
D providers/softlayer/src/test/java/org/jclouds/softlayer/parse/ParseProductOrderTest.java (105)
D providers/softlayer/src/test/java/org/jclouds/softlayer/parse/ParseVirtualGuestHaltedTest.java (73)
D providers/softlayer/src/test/java/org/jclouds/softlayer/parse/ParseVirtualGuestPausedTest.java (73)
D providers/softlayer/src/test/java/org/jclouds/softlayer/parse/ParseVirtualGuestRunningTest.java (73)
D providers/softlayer/src/test/java/org/jclouds/softlayer/parse/ParseVirtualGuestWithNoPasswordTest.java (69)
D providers/softlayer/src/test/java/org/jclouds/softlayer/predicates/ProductItemPredicatesTest.java (101)
D providers/softlayer/src/test/java/org/jclouds/softlayer/predicates/ProductPackagePredicatesTest.java (44)
A providers/softlayer/src/test/resources/datacenter_get.json (30)
A providers/softlayer/src/test/resources/datacenter_list.json (72)
D providers/softlayer/src/test/resources/log4j.xml (151)
A providers/softlayer/src/test/resources/logback.xml (34)
D providers/softlayer/src/test/resources/product_order_template.json (1337)
A providers/softlayer/src/test/resources/software_description_list.json (21)
D providers/softlayer/src/test/resources/virtual_guest_bad_halted.json (4)
A providers/softlayer/src/test/resources/virtual_guest_create.json (1)
A providers/softlayer/src/test/resources/virtual_guest_create_response.json (19)
A providers/softlayer/src/test/resources/virtual_guest_get.json (22)
D providers/softlayer/src/test/resources/virtual_guest_good_halted.json (117)
A providers/softlayer/src/test/resources/virtual_guest_list.json (24)
D providers/softlayer/src/test/resources/virtual_guest_no_password.json (74)
D providers/softlayer/src/test/resources/virtual_guest_paused.json (117)
D providers/softlayer/src/test/resources/virtual_guest_running.json (117)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/296.patch
https://github.com/jclouds/jclouds/pull/296.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true,
> + operatingSystemReferenceCode, null, localDiskFlag, new Datacenter(datacenterName), networkComponents,
> + getBlockDevices(virtualGuest));
> + } else if(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup() != null) {
> + String globalIdentifier = checkNotNull(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup()
> + .getGlobalIdentifier(), "blockDeviceTemplateGroup.globalIdentifier");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true, null,
> + new BlockDeviceTemplateGroup(globalIdentifier), localDiskFlag, new Datacenter(datacenterName),
> + networkComponents, null);
> + }
> + return json.toJson(ImmutableMap.of("parameters", ImmutableList.of(templateObject)));
> + }
> +
> + private Set<BlockDevice> getBlockDevices(VirtualGuest virtualGuest) {
> + if (virtualGuest.getVirtualGuestBlockDevices() == null) return null;
> + Set<BlockDevice> set = FluentIterable.from(virtualGuest.getVirtualGuestBlockDevices())
[minor] Rename `set` to `blockDevices`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324395
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + return processors;
> + }
> +
> + public Set<Integer> getCpusOfProcessors() {
> + if(processors.isEmpty()) return ImmutableSet.of();
> + return Sets.newHashSet(Iterables.transform(processors, new Function<ContainerVirtualGuestConfigurationOption,
> + Integer>() {
> + @Override
> + public Integer apply(ContainerVirtualGuestConfigurationOption input) {
> + return input.getTemplate().getStartCpus();
> + }
> + }));
> + }
> +
> + public Set<Integer> getMemories() {
> + if(memory.isEmpty()) return ImmutableSet.of();
Same as above.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882534
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> Are you suggesting a different workflow?
No, we can also go ahead this way and then "forward-port" the changes to master, of course ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36008741
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> @Consumes(MediaType.APPLICATION_JSON)
> - @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> - ProductPackage getProductPackage(@PathParam("id") long id);
> + @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> + Set<SoftwareDescription> getAllObjects();
> Let's see if anyone has a different opinion. @demobox?
Normally I'd probably say "use the provider APIs naming" as that is least surprising for the user looking for a certain call. But in this case, the provider's name is so unclear that I'd probably vote for `listSoftwareDescriptions` too, with some kind of comment in the Javadoc (or elsewhere?) explaining what the underlying call is.
Another aspect to consider, which I haven't hear, is some kind of consistency with respect to similar calls in this or related APIs. Is there anything we should be bearing in mind there?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r13061799
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -19,38 +19,36 @@
> /**
> * Configuration properties and constants used in SoftLayer connections.
> *
> - * @author Adrian Cole
> + * @author Adrian Cole, Andrea Turli
Same here. Each author in a separate line.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139488
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for the contribution Andrea!
I've started to look at it, but it is quite difficult to figure out what is changing from a functional perspective.
Could you please give a summary of the functional changes (how the compute integration worked and how it works now), if there are backwards incompatible changes (do people having code for the previous version experience issues/undesired behaviors when ipgrading?), and the result of the live tests?
Having a few lines explaining what changes will help us a lot understanding what's going on so we can properly review them.
Thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36067587
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + this.template = template;
> + return this;
> + }
> +
> + public ContainerVirtualGuestConfigurationOption build() {
> + return new ContainerVirtualGuestConfigurationOption(productItemPrice, template);
> + }
> +
> + public Builder fromContainerVirtualGuestConfigurationOption(ContainerVirtualGuestConfigurationOption in) {
> + return this
> + .productItemPrice(in.getProductItemPrice())
> + .template(in.getTemplate());
> + }
> + }
> +
> + private final ProductItemPrice productItemPrice;
Add missing `@Named("itemPrice")`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138511
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +package org.jclouds.softlayer.features;
> +
> +import com.google.common.collect.Iterables;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.http.HttpResponse;
> +import org.jclouds.softlayer.SoftLayerApi;
> +import org.jclouds.softlayer.parse.ListPublicImagesResponseTest;
> +import org.testng.annotations.Test;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertTrue;
> +
> +/**
> + * @author Andrea Turli
> + */
> +@Test(groups = "unit", testName = "AccountApiExpectTest")
Wrong `testName`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11140005
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -47,17 +47,17 @@ public DatacenterToLocation(JustProvider provider) {
>
> @Override
> public Location apply(Datacenter datacenter) {
> - return new LocationBuilder().scope(LocationScope.ZONE)
> - .metadata(ImmutableMap.<String, Object>of())
> + return new LocationBuilder().id(datacenter.getName())
Changing the `id` is a backward compatibility breaker. People using the ComputeService and the `TempalteBuilder.locationId()` will get errors as the location might no longer exist.
As this PR is opened against 1.7.x, could the previous ID be kept, and perhaps change it in master, if it is a must?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10275498
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #609](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/609/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-35519428
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #702](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/702/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-38732537
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> + @Nullable
> + public SoftwareDescription getSoftwareDescription() {
> + return this.softwareDescription;
> + }
> +
> + @Override
> + public int hashCode() {
> + return Objects.hashCode(id);
> + }
> +
> + @Override
> + public boolean equals(Object obj) {
> + if (this == obj) return true;
> + if (obj == null || getClass() != obj.getClass()) return false;
> + VirtualDiskImageSoftware that = VirtualDiskImageSoftware.class.cast(obj);
> + return Objects.equal(this.id, that.id);
not really. Thanks for spotting that
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11151800
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> Password pw = get(result.getOperatingSystem().getPasswords(), 0);
> return new NodeAndInitialCredentials<VirtualGuest>(result, result.getId() + "", LoginCredentials.builder().user(pw.getUsername()).password(
> pw.getPassword()).build());
> }
>
> - private Iterable<ProductItemPrice> getPrices(Template template) {
> - Builder<ProductItemPrice> result = ImmutableSet.builder();
> + private Set<VirtualGuestBlockDevice> getBlockDevices(List<Integer> blockDeviceCapacities) {
> + Set<VirtualGuestBlockDevice> blockDevices = Sets.newHashSet();
> + int devicePosition = 0;
> + for (int i = 0; i < blockDeviceCapacities.size(); i++) {
> + if (i > 0) { devicePosition = i + 1; }
This will lead to `devicePosition` being `0, 2, 3, 4...`. Is there a reason why 1 can't be used?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11137608
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #957](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/957/) UNSTABLE
Looks like there's a problem with this pull request
[(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/pull/296#issuecomment-38734577
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + }));
> + }
> +
> + public Set<Datacenter> getVirtualGuestDatacenters() {
> + if(datacenters.isEmpty()) return ImmutableSet.of();
> + return Sets.newHashSet(Iterables.transform(datacenters, new Function<ContainerVirtualGuestConfigurationOption,
> + Datacenter>() {
> + @Override
> + public Datacenter apply(ContainerVirtualGuestConfigurationOption input) {
> + return input.getTemplate().getDatacenter();
> + }
> + }));
> + }
> +
> + public Set<OperatingSystem> getVirtualGuestOperatingSystems() {
> + if(operatingSystems.isEmpty()) return ImmutableSet.of();
Same as above.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882541
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #971](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/971/) UNSTABLE
Looks like there's a problem with this pull request
[(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/pull/296#issuecomment-38820636
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> + String operatingSystemReferenceCode = checkNotNull(virtualGuest.getOperatingSystem()
> + .getOperatingSystemReferenceCode(), "operatingSystemReferenceCode");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true,
> + operatingSystemReferenceCode, null, localDiskFlag, new Datacenter(datacenterName), networkComponents,
> + getBlockDevices(virtualGuest));
> + } else if(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup() != null) {
> + String globalIdentifier = checkNotNull(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup()
> + .getGlobalIdentifier(), "blockDeviceTemplateGroup.globalIdentifier");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true, null,
> + new BlockDeviceTemplateGroup(globalIdentifier), localDiskFlag, new Datacenter(datacenterName),
> + networkComponents, null);
> + }
> + return json.toJson(ImmutableMap.of("parameters", ImmutableList.of(templateObject)));
> + }
> +
> + private Set<BlockDevice> getBlockDevices(VirtualGuest virtualGuest) {
I think this can be `private static`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324393
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + public Builder<?> toBuilder() {
> + return new ConcreteBuilder().fromVirtualGuestBlockDeviceTemplateGroup(this);
> + }
> +
> + public abstract static class Builder<T extends Builder<T>> {
> + protected abstract T self();
> +
> + protected int id;
> + protected String name;
> + protected String globalIdentifier;
> + protected int statusId;
> + protected int accountId;
> + protected int parentId;
> + protected String summary;
> + protected Set<VirtualGuestBlockDeviceTemplateGroup> children = ImmutableSet.of();
> + protected Set<VirtualGuestBlockDeviceTemplate> blockDevices = ImmutableSet.of();
Change to `ImmutableSet.Builder`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452221
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + private final int statusId;
> + private final String mountType;
> + private final String mountMode;
> + private final int bootableFlag;
> + private final String device;
> + private final VirtualDiskImage virtualDiskImage;
> + private final VirtualGuest guest;
> +
> + @ConstructorProperties({
> + "id", "uuid", "statusId", "mountType", "mountMode", "bootableFlag", "device", "diskImage", "guest"
> + })
> + protected VirtualGuestBlockDevice(int id, String uuid, int statusId, String mountType, String mountMode,
> + int bootableFlag, String device, VirtualDiskImage virtualDiskImage,
> + VirtualGuest guest) {
> + this.id = id;
> + this.uuid = uuid;
Sure! The `@Named` annotation is only needed in those fields which name differs from the serialized form.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10466914
Re: [jclouds] Refactor SoftLayer CCI support (#296)
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.jclouds.softlayer.features;
> +
> +import com.google.common.collect.Iterables;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.http.HttpResponse;
> +import org.jclouds.softlayer.parse.GetDatacenterResponseTest;
> +import org.jclouds.softlayer.parse.ListDatacentersResponseTest;
> +import org.testng.annotations.Test;
> +
> +import static org.testng.Assert.*;
Don't use wildcard imports.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139692
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> + boolean localDiskFlag = checkNotNull(virtualGuest.isLocalDiskFlag(), "localDiskFlag");
> + String datacenterName = checkNotNull(virtualGuest.getDatacenter().getName(), "datacenterName");
> + Set<NetworkComponent> networkComponents = getNetworkComponents(virtualGuest);
> + if(virtualGuest.getOperatingSystem() != null) {
> + String operatingSystemReferenceCode = checkNotNull(virtualGuest.getOperatingSystem()
> + .getOperatingSystemReferenceCode(), "operatingSystemReferenceCode");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true,
> + operatingSystemReferenceCode, null, localDiskFlag, new Datacenter(datacenterName), networkComponents,
> + getBlockDevices(virtualGuest));
> + } else if(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup() != null) {
> + String globalIdentifier = checkNotNull(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup()
> + .getGlobalIdentifier(), "blockDeviceTemplateGroup.globalIdentifier");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true, null,
> + new BlockDeviceTemplateGroup(globalIdentifier), localDiskFlag, new Datacenter(datacenterName),
> + networkComponents, null);
> + }
What if `getOperatingSystem` and `getVirtualGuestBlockDeviceTemplateGroup` are **both** `null`? Or is that not possible?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324390
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public Set<ContainerVirtualGuestConfigurationOption> getNetworkComponents() {
> + return networkComponents;
> + }
> +
> + public Set<ContainerVirtualGuestConfigurationOption> getOperatingSystems() {
> + return operatingSystems;
> + }
> +
> + public Set<ContainerVirtualGuestConfigurationOption> getProcessors() {
> + return processors;
> + }
> +
> + public Set<Integer> getCpusOfProcessors() {
> + if(processors.isEmpty()) return ImmutableSet.of();
By doing this you are returning an immutable set if there are no processors, but a mutable one if there is some. Why that different behavior? I'd just remove this line, as the `transform` below will already do nothing if the processors set is empty.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882529
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + * @author Andrea Turli
> + */
> +@Singleton
> +public class VirtualGuestToImage implements Function<VirtualGuest, Image> {
> +
> + private static final String UNRECOGNIZED = "UNRECOGNIZED";
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final OperatingSystemToImage operatingSystemToImage;
> +
> + @Inject
> + public VirtualGuestToImage(OperatingSystemToImage operatingSystemToImage) {
> + this.operatingSystemToImage = operatingSystemToImage;
Add a `checkNotNull` check here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882154
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -135,100 +148,129 @@ public SoftLayerComputeServiceAdapter(SoftLayerApi client,
>
> checkState(orderInSystem, "order for guest %s doesn't have login details within %sms", result,
> Long.toString(guestLoginDelay));
I know this check hasn't changed, but a failure at this point could leave the virtual guest created but the ComputeService won't return gracefully. Worth changing it to leave a log and not failing, or rolling back the server if it is not accessible? Just trying to avoid an inconsistent state or one where the user might think that the operation failed whilst the server remains created.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276329
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> + List<? extends Volume> volumes2 = h2.getVolumes();
> + ComparisonChain comparisonChain = ComparisonChain.start().compare(getCores(h1), getCores(h2))
> + .compare(h1.getRam(), h2.getRam())
> + .compare(getSpace(h1), getSpace(h2))
> + .compare(getBootableDeviceType(h1), getBootableDeviceType(h2));
> + if(!volumes1.isEmpty() && !volumes2.isEmpty() && volumes1.size() == volumes2.size()) {
> + for (int i = 0; i < volumes1.size(); i++) {
> + comparisonChain.compare(volumes1.get(i).getType(), volumes2.get(i).getType());
> + }
> + }
> + return comparisonChain.result();
> + }
> + });
> + for (Integer cpus : virtualGuestConfiguration.getCpusOfProcessors()) {
> + for (Integer memory : virtualGuestConfiguration.getMemories()) {
> + for (VirtualGuestBlockDevice blockDevice : virtualGuestConfiguration.getVirtualGuestBlockDevices()) {
yeah looks like a good optimization
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11150878
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> + public int compare(BlockDevice b1, BlockDevice b2) {
> + return Integer.valueOf(b1.getDevice()).compareTo(Integer.valueOf(b2.getDevice()));
> + }
> + });
> + return set;
> + }
> +
> + private HashSet<NetworkComponent> getNetworkComponents(VirtualGuest virtualGuest) {
> + if(virtualGuest.getVirtualGuestNetworkComponents() == null) return null;
> + return Sets.newHashSet(Iterables.transform(virtualGuest.getVirtualGuestNetworkComponents(),
> + new Function<VirtualGuestNetworkComponent, NetworkComponent>() {
> + @Override
> + public NetworkComponent apply(VirtualGuestNetworkComponent input) {
> + return new NetworkComponent(input.getSpeed());
> + }
> + }));
Again, unless Guava really helps here, just use:
```
ImmutableSet.Builder networkComponents = ImmutableSet.builder();
for (VGNC networkComponent : virtualGuest.getVirtualGuestNetworkComponents()) {
networkComponents.add(new NetworkComponent(input.getSpeed());
}
return networkComponents.build();
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324409
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + @Nullable String requiredUser, @Nullable String version, int controlPanel,
> + String upgradeSoftwareDescriptionId, String upgradeSwDescId, String virtualLicense,
> + String virtualizationPlatform) {
> + this.id = id;
> + this.longDescription = longDescription;
> + this.manufacturer = manufacturer;
> + this.name = name;
> + this.operatingSystem = operatingSystem;
> + this.referenceCode = referenceCode;
> + this.requiredUser = requiredUser;
> + this.version = version;
> + this.controlPanel = controlPanel;
> + this.upgradeSoftwareDescriptionId = upgradeSoftwareDescriptionId;
> + this.upgradeSwDescId = upgradeSwDescId;
> + this.virtualLicense = virtualLicense;
> + this.virtualizationPlatform = virtualizationPlatform;
Add null checks for all non-nullable properties.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138792
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #854](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/854/) UNSTABLE
Looks like there's a problem with this pull request
[(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/pull/296#issuecomment-35520263
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> /**
> - * Power on a guest
> + * hard reboot the guest.
Minor: start in upper case.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452537
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> - assert vg.getMetricPollDate() != null : vg;
> - assert vg.getModifyDate() != null : vg;
> - assert vg.getStartCpus() > 0 : vg;
> - assert vg.getStatusId() >= 0 : vg;
> - assert vg.getUuid() != null : vg;
> - assert vg.getPrimaryBackendIpAddress() != null : vg;
> - assert vg.getPrimaryIpAddress() != null : vg;
> + if (vg.getActiveTransactionCount() == 0) {
> + assertNotNull(vg.getDomain(), "domain must be not null");
> + assertNotNull(vg.getFullyQualifiedDomainName(), "fullyQualifiedDomainName must be not null");
> + assertNotNull(vg.getHostname(), "hostname must be not null");
> + assertTrue(vg.getId() > 0, "id must be greater than 0");
> + assertTrue(vg.getMaxCpu() > 0, "maxCpu must be greater than 0");
> + assertTrue(vg.getMaxMemory() > 0, "maxMemory must be greater than 0");
> + assertTrue(vg.getStatusId() > 0, "statusId must be greater than 0");
> + }
> }
Also add live tests for the pause, reboot and resume operations.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882941
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> * @see <a href="http://sldn.softlayer.com/article/REST" />
> - * @author Adrian Cole
> + * @author Adrian Cole, Andrea Turli
Same here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139301
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> })
> - protected Address(int id, String country, @Nullable String state, @Nullable String description) {
> + protected Address(int id, String country, @Nullable String state, @Nullable String description, @Nullable int accountId,
> + @Nullable String address1, @Nullable String city, @Nullable String contactName,
> + @Nullable int isActive, @Nullable int locationId, @Nullable String postalCode) {
> this.id = id;
Add null check to the id field too
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276758
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> - assert vg.getStatusId() >= 0 : vg;
> - assert vg.getUuid() != null : vg;
> - assert vg.getPrimaryBackendIpAddress() != null : vg;
> - assert vg.getPrimaryIpAddress() != null : vg;
> + if (vg.getActiveTransactionCount() == 0) {
> + assertNotNull(vg.getDomain(), "domain must be not null");
> + assertNotNull(vg.getFullyQualifiedDomainName(), "fullyQualifiedDomainName must be not null");
> + assertNotNull(vg.getHostname(), "hostname must be not null");
> + assertTrue(vg.getId() > 0, "id must be greater than 0");
> + assertTrue(vg.getMaxCpu() > 0, "maxCpu must be greater than 0");
> + assertNotNull(vg.getMaxCpuUnits(), "maxCpuUnits must be not null");
> + assertTrue(vg.getMaxMemory() > 0, "maxMemory must be greater than 0");
> + assertTrue(vg.getStartCpus() > 0, "startCpus must be greater than 0");
> + assertTrue(vg.getStatusId() > 0, "statusId must be greater than 0");
> + assertNotNull(vg.getUuid(), "uuid must be not null");
> + }
> }
>
> }
Still missing many live test methods. Currently only the get and create api methods are being tested.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139941
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + private final int statusId;
> + private final String mountType;
> + private final String mountMode;
> + private final int bootableFlag;
> + private final String device;
> + private final VirtualDiskImage virtualDiskImage;
> + private final VirtualGuest guest;
> +
> + @ConstructorProperties({
> + "id", "uuid", "statusId", "mountType", "mountMode", "bootableFlag", "device", "diskImage", "guest"
> + })
> + protected VirtualGuestBlockDevice(int id, String uuid, int statusId, String mountType, String mountMode,
> + int bootableFlag, String device, VirtualDiskImage virtualDiskImage,
> + VirtualGuest guest) {
> + this.id = id;
> + this.uuid = uuid;
Add a null check for all required parameters, or mark them as `@Nullable`.
Also annotate each class member variable which name does not match the serialized representation with: `@Named("serialized_name")`. This way, the `@ConstructorProperties` tells jclouds how to deserialize the object, and the `@Named` annotations will tell how to serialize, if needed (whether domain objects might not be directly serialized, annotations should be consistent).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452016
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + String buildJson(VirtualGuest virtualGuest) {
> + TemplateObject templateObject = null;
> + String hostname = checkNotNull(virtualGuest.getHostname(), "hostname");
> + String domain = checkNotNull(virtualGuest.getDomain(), "domain");
> + int startCpus = checkNotNull(virtualGuest.getStartCpus(), "startCpus");
> + int maxMemory = checkNotNull(virtualGuest.getMaxMemory(), "maxMemory");
> + boolean localDiskFlag = checkNotNull(virtualGuest.isLocalDiskFlag(), "localDiskFlag");
> + String datacenterName = checkNotNull(virtualGuest.getDatacenter().getName(), "datacenterName");
> + Set<NetworkComponent> networkComponents = getNetworkComponents(virtualGuest);
> + if(virtualGuest.getOperatingSystem() != null) {
> + String operatingSystemReferenceCode = checkNotNull(virtualGuest.getOperatingSystem()
> + .getOperatingSystemReferenceCode(), "operatingSystemReferenceCode");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true,
> + operatingSystemReferenceCode, null, localDiskFlag, new Datacenter(datacenterName), networkComponents,
> + getBlockDevices(virtualGuest));
> + } else if(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup() != null) {
[minor] space after `if`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11136416
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #698](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/698/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-38707154
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +@Singleton
> +public class VirtualGuestToImage implements Function<VirtualGuest, Image> {
> +
> + private static final String CENTOS = "CENTOS";
> + private static final String DEBIAN = "DEBIAN";
> + private static final String RHEL = "REDHAT";
> + private static final String UBUNTU = "UBUNTU";
> + private static final String WINDOWS = "WIN_";
> + private static final String CLOUD_LINUX = "CLOUDLINUX";
> + private static final String VYATTACE = "VYATTACE";
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + public static Function<String, OsFamily> osFamily() {
Can we remove this function and reuse the one in the `OperatingSystemToImage` class?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11136853
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + */
> + public T summary(String summary) {
> + this.summary = summary;
> + return self();
> + }
> +
> + /**
> + * @see VirtualGuestBlockDeviceTemplateGroup#getChildren()
> + */
> + public T children(Set<VirtualGuestBlockDeviceTemplateGroup> children) {
> + this.children = ImmutableSet.copyOf(checkNotNull(children, "children"));
> + return self();
> + }
> +
> + public T children(VirtualGuestBlockDeviceTemplateGroup... in) {
> + return children(ImmutableSet.copyOf(in));
Also check for the `null` input here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452263
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> + private final int statusId;
> + private final String mountType;
> + private final String mountMode;
> + private final int bootableFlag;
> + private final String device;
> + private final VirtualDiskImage virtualDiskImage;
> + private final VirtualGuest guest;
> +
> + @ConstructorProperties({
> + "id", "uuid", "statusId", "mountType", "mountMode", "bootableFlag", "device", "diskImage", "guest"
> + })
> + protected VirtualGuestBlockDevice(int id, String uuid, int statusId, String mountType, String mountMode,
> + int bootableFlag, String device, VirtualDiskImage virtualDiskImage,
> + VirtualGuest guest) {
> + this.id = id;
> + this.uuid = uuid;
thanks for the clarification, but I think in my case both `@ConstructorProperties` and `@Named` would be redundant most of the times because the filed name almost always matches the json field name
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10466708
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class Tag {
> + private final int accountId;
> + private final int id;
> + private final int internal;
> + private final String name;
> +
> + @ConstructorProperties({"accountId", "id", "internal", "name"} )
> + public Tag(int accountId, int id, int internal, String name) {
> + this.accountId = checkNotNull(accountId, "accountId");
> + this.id = checkNotNull(id, "id");
> + this.internal = checkNotNull(internal, "internal");
Remove null check for primitives.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882660
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> if (image != null) {
> builder.imageId(image.getId());
> builder.operatingSystem(image.getOperatingSystem());
> }
> -
> - builder.hardware(hardware.getHardware(from));
> -
> - builder.status(serverStateToNodeStatus.get(from.getPowerState().getKeyName()));
> -
> - // These are null for 'bad' guest orders in the HALTED state.
> + if(from.getPowerState() != null) {
[minor] space after the if.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882232
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + // multi-disk option
> + options.blockDevices(ImmutableList.of(100, 400, 400));
> +
> + Set<? extends NodeMetadata> nodes = context.getComputeService().createNodesInGroup(name, numNodes, template);
> + assertEquals(numNodes, nodes.size(), "wrong number of nodes");
> + for (NodeMetadata node : nodes) {
> + logger.debug("Created Node: %s", node);
> + SshClient client = context.utils().sshForNode().apply(node);
> + client.connect();
> + ExecResponse hello = client.exec("mount");
> + System.out.println(hello.getOutput().trim());
> +
> + VirtualGuest virtualGuest = context.unwrapApi(SoftLayerApi.class).getVirtualGuestApi()
> + .getObject(Long.parseLong(node.getId()));
> + for (VirtualGuestBlockDevice blockDevice : virtualGuest.getVirtualGuestBlockDevices()) {
> + System.out.println(blockDevice);
Same here. Use the logger.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139608
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1168](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1168/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-38707818
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> - @ConstructorProperties({
> - "id", "description", "units", "capacity", "prices", "categories"
> - })
> - protected ProductItem(int id, @Nullable String description, @Nullable String units, @Nullable Float capacity, @Nullable Set<ProductItemPrice> prices, @Nullable Set<ProductItemCategory> categories) {
> + public ProductItem(int id, String description, String softwareDescriptionId, SoftwareDescription softwareDescription) {
Missing the `@ConstructorProperties` annotation?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138678
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
Hi Ignasi,
I think you are right, a little of context could definitely ease the review.
Recently I've been working with SoftLayer and I tried to fix some blocking
issue (PR#200, PR#204) but unfortunately they didn't fix them all.
In particular VirtualGuestToNodeMetadata happens to be slow and buggy and
most of the times I couldn't listNodes particularly when the account
contains nodes created manually (not by jclouds)
Also a couple of threads [1], [2], [3] on jclouds-users convinced me to
look better at the code to simplify the current impl so that maintenance
could be easier.
Looking for a better design of jclouds softlayer, I stumbled upon
soflayer-python [4] which uses a completely (simplified!) set of api calls
[5] to deal with SoftLayer CCI than the ones jclouds uses.
It seems to me, in fact, that when the first version of SoftLayer provider
was written either those simplified api calls where not there or maybe the
intent was to have a very generic softlayer provider to support multiple
product offered by SoftLayer.
The original implementation tries to filter from all the ProductPackage
supported the VIRTUALGUEST_PACKAGE and then to create a VirtualGuest (aka
CCI) composing a ProductOrder and place a productOrder. This is probably a
more generic approach than dealing only with SoftLayer CCI tailored API but
it is demonstrated to be hard to understand, hard improve and, above all,
hard to make it right.
My refactoring has removed the concept of productOrder and deals directly
with VirtualGuest API: these have a number of advantages, like
hardwareProfiles are now effectively built on getCreateObjectOptions [6]
that are the only supported options for the VirtualGuests, as well as
listAvailableLocations, not mentioning createNode of course.
In terms of code, I've modernized it removing the asyncApi and refactoring
expectTests and LiveTests.
Hope this helps, but as it is not easy to explain all at once, feel free to
ask me more info either here on IRC channel.
Thanks!
Andrea
[1]: http://www.mail-archive.com/user@jclouds.apache.org/msg00421.html
[2]: http://www.mail-archive.com/user@jclouds.apache.org/msg00043.html
[3]: http://www.mail-archive.com/user@jclouds.apache.org/msg00085.html
[4]: https://github.com/softlayer/softlayer-python
[5]:
http://sldn.softlayer.com/reference/services/SoftLayer_Virtual_Guest/createObject
[6]:
http://sldn.softlayer.com/reference/services/SoftLayer_Virtual_Guest/getCreateObjectOptions
Il 25/feb/2014 23:36 "Ignasi Barrera" <no...@github.com> ha scritto:
> Thanks for the contribution Andrea!
>
> I've started to look at it, but it is quite difficult to figure out what
> is changing from a functional perspective.
>
> Could you please give a summary of the functional changes (how the compute
> integration worked and how it works now), if there are backwards
> incompatible changes (do people having code for the previous version
> experience issues/undesired behaviors when ipgrading?), and the result of
> the live tests?
>
> Having a few lines explaining what changes will help us a lot
> understanding what's going on so we can properly review them.
>
> Thx!
>
> --
> Reply to this email directly or view it on GitHub<https://github.com/jclouds/jclouds/pull/296#issuecomment-36067587>
> .
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36102733
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +/**
> + * @author Andrea Turli
> + */
> +@Singleton
> +public class VirtualGuestToImage implements Function<VirtualGuest, Image> {
> +
> + private static final String UNRECOGNIZED = "UNRECOGNIZED";
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final OperatingSystemToImage operatingSystemToImage;
> +
> + @Inject
> + public VirtualGuestToImage(OperatingSystemToImage operatingSystemToImage) {
Make the constructor package protected by removing the `public` modifier.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882191
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.base.Splitter;
> +import com.google.common.collect.Iterables;
> +
> +import static com.google.common.collect.Iterables.getLast;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class OperatingSystems {
> +
> + public static Function<String, Integer> bits() {
> + return new Function<String, Integer>() {
> + @Override
> + public Integer apply(String operatingSystemReferenceCode) {
> + if (operatingSystemReferenceCode != null) {
> + return Integer.parseInt(getLast(Splitter.on("_").split(operatingSystemReferenceCode)));
If the function is allowed to return null, perhaps it is better to use `Ints.tryParse` instead of `parseInt` to avoid throwing an exception and return null instead.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11137001
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #890](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/890/) 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/pull/296#issuecomment-36765227
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> + if(optVersion.isPresent()) {
> + osVersion = OperatingSystems.version().apply(optVersion.get());
> + }
> + if (osFamily == OsFamily.UNRECOGNIZED) {
> + logger.debug("Cannot determine os family for item: %s", operatingSystem);
> + }
> + if (osVersion == null) {
> + logger.debug("Cannot determine os version for item: %s", operatingSystem);
> + }
> + if (bits == null) {
> + logger.debug("Cannot determine os bits for item: %s", operatingSystem);
> + }
> +
> + org.jclouds.compute.domain.OperatingSystem os = org.jclouds.compute.domain.OperatingSystem.builder()
> + .description(optLongDescription.isPresent() ? optLongDescription.get() : UNRECOGNIZED)
Use just `optLongDescription.or(UNRECOGNIZED)`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882103
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> import org.jclouds.compute.ComputeServiceContext;
> import org.jclouds.rest.internal.BaseHttpApiMetadata;
> +import org.jclouds.rest.internal.BaseRestApiMetadata;
Remove the unused import
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10274809
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> builder.group(nodeNamingConvention.groupInUniqueNameOrNull(from.getHostname()));
> -
> - Image image = images.getImage(from);
> + builder.hardware(new VirtualGuestToHardware().apply(from));
Inject this function in the constructor
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882214
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + private final Set<VirtualGuestBlockDeviceTemplateGroup> children;
> + private final Set<VirtualGuestBlockDeviceTemplate> blockDevices;
> +
> + @ConstructorProperties({
> + "id", "name", "globalIdentifier", "statusId", "accountId", "parentId", "summary", "children", "blockDevices"
> + })
> + protected VirtualGuestBlockDeviceTemplateGroup(int id, @Nullable String name, @Nullable String globalIdentifier,
> + int statusId, int accountId, int parentId, @Nullable String summary,
> + @Nullable Set<VirtualGuestBlockDeviceTemplateGroup> children,
> + @Nullable Set<VirtualGuestBlockDeviceTemplate> blockDevices) {
> + this.id = checkNotNull(id, "id");
> + this.name = name;
> + this.globalIdentifier = globalIdentifier;
> + this.statusId = checkNotNull(statusId, "statusId");
> + this.accountId = checkNotNull(accountId, "accountId");
> + this.parentId = checkNotNull(parentId, "parentId");
Again here, primitives can't be null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139169
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + private static String parseVersion(String version) {
> + if(version.contains("-")) {
> + String rawVersion = version.substring(0,
> + version.lastIndexOf("-"));
> + if(Iterables.size(Splitter.on(".").split(rawVersion)) == 3) {
> + return rawVersion.substring(0, rawVersion.lastIndexOf("."));
> + } else {
> + return rawVersion;
> + }
> + } else if(version.contains(" ")) {
> + version.substring(0,
> + version.indexOf(" "));
> + }
> + return null;
> + }
The same applies here. Reuse the functions instead of duplicating them.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10275890
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> +import org.jclouds.softlayer.domain.VirtualGuest;
> +import org.jclouds.softlayer.domain.VirtualGuestBlockDevice;
> +
> +import javax.inject.Inject;
> +import java.util.HashSet;
> +import java.util.Set;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * Converts a VirtualGuest into a json string valid for creating a CCI via softlayer api
> + * The string is set into the payload of the HttpRequest
> + *
> + * @author Andrea Turli
> + */
> +public class VirtualGuestToJson implements Binder {
I agree with you but in the jclouds philosophy I think VirtualGuestApi should reflect the signature of the REST api [createObject](http://sldn.softlayer.com/reference/services/SoftLayer_Virtual_Guest/createObject) so in other words it needs to get the SoftLayer_Virtual_Guest object that you wish to create.
I'm not particular happy of this design decision done at SoftLayer but I think it is what it is.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10298946
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + // list private images and transform them to OperatingSystem
> + Set<VirtualGuestBlockDeviceTemplateGroup> privateImages = api.getAccountApi().getBlockDeviceTemplateGroups();
> + Map<String, SoftwareDescription> privateImagesSoftwareDescriptions = extractSoftwareDescriptions(privateImages);
> + for (Map.Entry<String, SoftwareDescription> entry : privateImagesSoftwareDescriptions.entrySet()) {
> + OperatingSystem os = getOperatingSystem(entry);
> + if (os != null) {
> + result.add(os);
> + }
> + }
> + // list public images and transform them to OperatingSystem
> + if(includePublicImages) {
> + Set<VirtualGuestBlockDeviceTemplateGroup> publicImages = api.getVirtualGuestBlockDeviceTemplateGroupApi().getPublicImages();
> + Map<String, SoftwareDescription> publicImagesSoftwareDescriptions = extractSoftwareDescriptions(publicImages);
> + for (Map.Entry<String, SoftwareDescription> entry :
> + publicImagesSoftwareDescriptions.entrySet()) {
[minor] Remove the line break and add spaces after all `if` statements.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138078
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> */
> @Delegate
> - ProductPackageApi getProductPackageApi();
We started the review of this PR opened against 1.7.x, but as it is not backwards compatible we agreed to finish the review here, to keep the context, and once it is OK this PR will be closed and reopened against master.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12881992
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> logger.debug(">> awaiting login details for virtualGuest(%s)", result.getId());
> boolean orderInSystem = loginDetailsTester.apply(result);
> - logger.trace("<< virtualGuest(%s) complete(%s)", result.getId(), orderInSystem);
> + logger.trace("<< VirtualGuest(%s) complete(%s)", result.getId(), orderInSystem);
> +
> + if(!orderInSystem) {
[minor] Space after the if.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882325
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +@Singleton
> +public class VirtualGuestToImage implements Function<VirtualGuest, Image> {
> +
> + private static final String CENTOS = "CENTOS";
> + private static final String DEBIAN = "DEBIAN";
> + private static final String RHEL = "REDHAT";
> + private static final String UBUNTU = "UBUNTU";
> + private static final String WINDOWS = "WIN_";
> + private static final String CLOUD_LINUX = "CLOUDLINUX";
> + private static final String VYATTACE = "VYATTACE";
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + public static Function<String, OsFamily> osFamily() {
This seems the same function than the one defined in the `OperatingSystemToImage`. Can we have only one and reuse it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10275858
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> @@ -197,20 +181,4 @@ public SoftLayerTemplateOptions userMetadata(Map<String, String> userMetadata) {
> public SoftLayerTemplateOptions userMetadata(String key, String value) {
> return SoftLayerTemplateOptions.class.cast(super.userMetadata(key, value));
> }
> -
> - /**
> - * {@inheritDoc}
> - */
> - @Override
> - public SoftLayerTemplateOptions nodeNames(Iterable<String> nodeNames) {
> - return SoftLayerTemplateOptions.class.cast(super.nodeNames(nodeNames));
> - }
Sorry, probably a mistake. thanks for spotting it
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10299493
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + .compare(getSpace(h1), getSpace(h2))
> + .compare(getBootableDeviceType(h1), getBootableDeviceType(h2));
> + if(!volumes1.isEmpty() && !volumes2.isEmpty() && volumes1.size() == volumes2.size()) {
> + for (int i = 0; i < volumes1.size(); i++) {
> + comparisonChain.compare(volumes1.get(i).getType(), volumes2.get(i).getType());
> + }
> + }
> + return comparisonChain.result();
> + }
> + });
> + for (VirtualGuestBlockDevice blockDevice : virtualGuestConfiguration.getVirtualGuestBlockDevices()) {
> + if (blockDevice.getDevice().equals(BOOTABLE_DEVICE)) {
> + for (Integer cpus : virtualGuestConfiguration.getCpusOfProcessors()) {
> + for (Integer memory : virtualGuestConfiguration.getMemories()) {
> + float capacity = blockDevice.getVirtualDiskImage().getCapacity();
> + Type type = blockDevice.getVirtualGuest().isLocalDiskFlag() ? Type.LOCAL : Type.SAN;
Move these two lines outside the for loops to compute them only once.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882384
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + this.softwareDescriptionId = softwareDescriptionId;
> + }
> +
> + public int getId() {
> + return this.id;
> + }
> +
> + @Nullable
> + public SoftwareDescription getSoftwareDescription() {
> + return this.softwareDescription;
> + }
> +
> + /**
> + * @return A longer location description.
> + */
> + @Nullable
Primitive types can't be null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138825
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #646](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/646/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-37188886
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> Will continue reviewing tomorrow
Thanks for all the review hours, @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36737788
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #997](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/997/) 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/pull/296#issuecomment-39556973
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + /**
> + * Builds a Json string suitable for sending to the softlayer api
> + *
> + * @param virtualGuest
> + * @return String
> + */
> + String buildJson(VirtualGuest virtualGuest) {
> + TemplateObject templateObject = null;
> + String hostname = checkNotNull(virtualGuest.getHostname(), "hostname");
> + String domain = checkNotNull(virtualGuest.getDomain(), "domain");
> + int startCpus = checkNotNull(virtualGuest.getStartCpus(), "startCpus");
> + int maxMemory = checkNotNull(virtualGuest.getMaxMemory(), "maxMemory");
> + boolean localDiskFlag = checkNotNull(virtualGuest.isLocalDiskFlag(), "localDiskFlag");
> + String datacenterName = checkNotNull(virtualGuest.getDatacenter().getName(), "datacenterName");
Can the `virtualGuest.getDatacenter()` be null?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11136382
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> import com.google.inject.AbstractModule;
>
> /**
> *
> - * @author Adrian Cole
> + * @author Adrian Cole, Andrea Turli
Add each author in a different line.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276584
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> */
> public class ProductItemPrice {
>
> - public static Builder<?> builder() {
> - return new ConcreteBuilder();
> + private final int id;
> + private final float hourlyRecurringFee;
> + private final String recurringFee;
> + private final ProductItem item;
> +
> + @ConstructorProperties({"id", "hourlyRecurringFee", "recurringFee", "item"})
> + public ProductItemPrice(int id, float hourlyRecurringFee, String recurringFee, ProductItem item) {
> + this.id = checkNotNull(id, "id");
Same comment. Primitives can't be null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882600
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + //templateBuilder.minRam(8192);
> + Template template = templateBuilder.build();
> + // test passing custom options
> + SoftLayerTemplateOptions options = template.getOptions().as(SoftLayerTemplateOptions.class);
> + options.domainName("live.org");
> + // multi-disk option
> + options.blockDevices(ImmutableList.of(100, 400, 400));
> +
> + Set<? extends NodeMetadata> nodes = context.getComputeService().createNodesInGroup(name, numNodes, template);
> + assertEquals(numNodes, nodes.size(), "wrong number of nodes");
> + for (NodeMetadata node : nodes) {
> + logger.debug("Created Node: %s", node);
> + SshClient client = context.utils().sshForNode().apply(node);
> + client.connect();
> + ExecResponse hello = client.exec("mount");
> + System.out.println(hello.getOutput().trim());
Use the logger.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139594
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.softlayer.domain.Datacenter;
> +import org.jclouds.softlayer.domain.OperatingSystem;
> +import org.jclouds.softlayer.domain.SoftwareDescription;
> +import org.jclouds.softlayer.domain.SoftwareLicense;
> +import org.jclouds.softlayer.domain.VirtualGuest;
> +import org.testng.annotations.Test;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +
> +/**
> + * Tests the function that transforms SoftLayer VirtualGuest to generic image.
> + *
> + * @author Andrea Turli
> + */
> +@Test(testName = "VirtualGuestToImageTest")
Add `groups="unit"`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882841
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.softlayer.domain.VirtualGuest;
> +import org.jclouds.softlayer.domain.VirtualGuestBlockDevice;
> +
> +import javax.inject.Inject;
> +import java.util.HashSet;
> +import java.util.Set;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * Converts a VirtualGuest into a json string valid for creating a CCI via softlayer api
> + * The string is set into the payload of the HttpRequest
> + *
> + * @author Andrea Turli
> + */
> +public class VirtualGuestToJson implements Binder {
This binding is only used when creating a `VirtualGuest`, to bind the VirtualGuest parameter to the request body.
The VirtualGuest object, however, has many fields that are not required in the create operation and the binder just extracts the required fields, populates them into a "smaller" object, and then binds it to the request body.
This approach seems a bit confusing, as it might be difficult for users to know which fields of the VirtualGuest object have to be populated in order to create it. Could the api method be refactored to directly receive the `ObjectData` (perhaps renamed to VirtualGuestCreation or similar) instead of the entire VirtualGuest? WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10275204
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1172](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1172/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-38733101
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> */
> public class ProductItem {
>
> - public static Builder<?> builder() {
> - return new ConcreteBuilder();
> + private final int id;
> + private final String description;
> + private final String softwareDescriptionId;
> + private final SoftwareDescription softwareDescription;
> +
> + @ConstructorProperties({"id", "description", "softwareDescriptionId", "softwareDescription"})
> + public ProductItem(int id, @Nullable String description, @Nullable String softwareDescriptionId,
> + @Nullable SoftwareDescription softwareDescription) {
> + this.id = checkNotNull(id, "id");
A primitive `int` will never be null, you could remove this check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882589
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> Password pw = get(result.getOperatingSystem().getPasswords(), 0);
> return new NodeAndInitialCredentials<VirtualGuest>(result, result.getId() + "", LoginCredentials.builder().user(pw.getUsername()).password(
> pw.getPassword()).build());
> }
>
> - private Iterable<ProductItemPrice> getPrices(Template template) {
> - Builder<ProductItemPrice> result = ImmutableSet.builder();
> + private Set<VirtualGuestBlockDevice> getBlockDevices(List<Integer> blockDeviceCapacities) {
> + Set<VirtualGuestBlockDevice> blockDevices = Sets.newHashSet();
> + int devicePosition = 0;
> + for (int i = 0; i < blockDeviceCapacities.size(); i++) {
> + if (i > 0) { devicePosition = i + 1; }
Right, devicePosition 1 is reserved to SWAP device
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11150799
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> */
> @Singleton
> public class SoftLayerComputeServiceAdapter implements
> - ComputeServiceAdapter<VirtualGuest, Iterable<ProductItem>, ProductItem, Datacenter> {
> + ComputeServiceAdapter<VirtualGuest, Hardware, OperatingSystem, Datacenter> {
> +
> + private static final String BOOTABLE_DEVICE = "0";
> + public static final String DEFAULT_DISK_TYPE = "LOCAL";
> + public static final int DEFAULT_PORT_SPEED = 100;
> + private static final boolean INCLUDE_PUBLIC_IMAGE = false;
This variable is not used.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882264
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #640](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/640/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36763465
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> /**
> * @author Adrian Cole
> */
> @Test(groups = "unit", testName = "VirtualGuestToNodeMetadataTest")
> public class VirtualGuestToNodeMetadataTest {
> +
> + // TODO
> + /*
I think this test doesn't make sense anymore as the VirtualGuestToNodeMetadata is now really simpler.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10490952
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -50,6 +49,13 @@
> protected String country;
> protected String state;
> protected String description;
> + protected int accountId;
> + protected String address1;
Can this be renamed to just `address` in the builder and domain object? You can use the `@Named` and `@ConstructorProperties` to specify its serialized form.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276711
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1230](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1230/) 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/pull/296#issuecomment-46187730
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #699](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/699/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-38707059
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> public class SoftLayerHttpApiModule extends HttpApiModule<SoftLayerApi> {
>
> + public SoftLayerHttpApiModule() {
> + }
This constructor isn't needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138265
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @Consumes(MediaType.APPLICATION_JSON)
> - @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
> - void rebootHardVirtualGuest(@PathParam("id") long id);
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + boolean deleteObject(@PathParam("id") long id);
Here also, `deleteVirtualGuest`. Apply this in general to all the methods in this API class that refer to VirtualGuests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452564
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -107,13 +176,30 @@ protected ConcreteBuilder self() {
> private final String country;
> private final String state;
> private final String description;
> + private final int accountId;
> + private final String address;
Add missing `@Named("address1")`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138342
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> + boolean localDiskFlag = checkNotNull(virtualGuest.isLocalDiskFlag(), "localDiskFlag");
> + String datacenterName = checkNotNull(virtualGuest.getDatacenter().getName(), "datacenterName");
> + Set<NetworkComponent> networkComponents = getNetworkComponents(virtualGuest);
> + if(virtualGuest.getOperatingSystem() != null) {
> + String operatingSystemReferenceCode = checkNotNull(virtualGuest.getOperatingSystem()
> + .getOperatingSystemReferenceCode(), "operatingSystemReferenceCode");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true,
> + operatingSystemReferenceCode, null, localDiskFlag, new Datacenter(datacenterName), networkComponents,
> + getBlockDevices(virtualGuest));
> + } else if(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup() != null) {
> + String globalIdentifier = checkNotNull(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup()
> + .getGlobalIdentifier(), "blockDeviceTemplateGroup.globalIdentifier");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true, null,
> + new BlockDeviceTemplateGroup(globalIdentifier), localDiskFlag, new Datacenter(datacenterName),
> + networkComponents, null);
> + }
it can't happen as an `imageId` is always passed and so operating system or a blockDeviceTemplateGroup will be always set
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r13794555
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> @Consumes(MediaType.APPLICATION_JSON)
> - @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> - ProductPackage getProductPackage(@PathParam("id") long id);
> + @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> + Set<SoftwareDescription> getAllObjects();
@nacx this confuses me as well: the SoftLayer api is named `getAllObjects` but it is actually `listSoftwareDescriptions`. Shall I use the real name or the meaningful one? thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10468186
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #894](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/894/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-46186880
Re: [jclouds] Refactor SoftLayer CCI support (#296)
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.jclouds.softlayer.domain;
> +
> +import com.google.common.base.Objects;
> +
> +/**
> + * @author Andrea Turli
> + * @see <a href="http://sldn.softlayer.com/reference/datatypes/SoftLayer_Provisioning_Version1_Transaction"/>
> + */
> +public class ProvisioningVersion1Transaction {
This class seems to be unused. Can it be removed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882626
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1105](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1105/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36625273
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import org.jclouds.compute.domain.Image;
> +import org.jclouds.softlayer.domain.OperatingSystem;
> +import org.jclouds.softlayer.domain.SoftwareDescription;
> +import org.jclouds.softlayer.domain.SoftwareLicense;
> +import org.testng.annotations.Test;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertTrue;
> +
> +/**
> + * Tests the function that transforms SoftLayer OperatingSystem to generic image.
> + *
> + * @author Andrea Turli
> + */
> +@Test(testName = "OperatingSystemToImageTest")
Add `groups="unit"`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882827
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> /**
> * @author Adrian Cole
> */
> @Test(groups = "unit", testName = "VirtualGuestToNodeMetadataTest")
> public class VirtualGuestToNodeMetadataTest {
> +
> + // TODO
> + /*
This entire class is commented out. Can we enable these tests?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10453056
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> */
> -@Test(groups = "unit", testName = "VirtualGuestToNodeMetadataTest")
> +@Test(testName = "VirtualGuestToNodeMetadataTest")
Add `groups="unit"`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882844
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> */
> -@ConfiguresRestClient
> -public class SoftLayerHttpApiModule extends HttpApiModule<SoftLayerApi> {
> +@ConfiguresHttpApi
> +public class SoftLayerRestClientModule extends HttpApiModule<SoftLayerApi> {
Rename to `SoftLayerHttpApiModule`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276627
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> - assert vg.getStatusId() >= 0 : vg;
> - assert vg.getUuid() != null : vg;
> - assert vg.getPrimaryBackendIpAddress() != null : vg;
> - assert vg.getPrimaryIpAddress() != null : vg;
> + if (vg.getActiveTransactionCount() == 0) {
> + assertNotNull(vg.getDomain(), "domain must be not null");
> + assertNotNull(vg.getFullyQualifiedDomainName(), "fullyQualifiedDomainName must be not null");
> + assertNotNull(vg.getHostname(), "hostname must be not null");
> + assertTrue(vg.getId() > 0, "id must be greater than 0");
> + assertTrue(vg.getMaxCpu() > 0, "maxCpu must be greater than 0");
> + assertNotNull(vg.getMaxCpuUnits(), "maxCpuUnits must be not null");
> + assertTrue(vg.getMaxMemory() > 0, "maxMemory must be greater than 0");
> + assertTrue(vg.getStartCpus() > 0, "startCpus must be greater than 0");
> + assertTrue(vg.getStatusId() > 0, "statusId must be greater than 0");
> + assertNotNull(vg.getUuid(), "uuid must be not null");
> + }
> }
>
> }
Same here. Add live methods for all the methods in the VirtualGuestApi
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10453249
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @QueryParams(keys = "objectMask", values = GUEST_MASK)
> @Consumes(MediaType.APPLICATION_JSON)
> @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> - VirtualGuest getVirtualGuest(@PathParam("id") long id);
> + VirtualGuest getObject(@PathParam("id") long id);
Rename to `getVirtualGuest`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452490
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1127](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1127/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-37484380
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + * Class VirtualDiskImage
> + *
> + * @author Andrea Turli
> + * @see <a href= "http://sldn.softlayer.com/reference/datatypes/SoftLayer_Virtual_Disk_Image"/>
> + */
> +public class VirtualDiskImage {
> +
> + public static Builder<?> builder() {
> + return new ConcreteBuilder();
> + }
> +
> + public Builder<?> toBuilder() {
> + return new ConcreteBuilder().fromVirtualDiskImage(this);
> + }
> +
> + public abstract static class Builder<T extends Builder<T>> {
If the builder is not going to be subclassed, don't make it abstract and remove the `ConcreteBuilder`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882692
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
By the way, is there any reason this is being submitted as a PR against 1.7.x, rather than master?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-35975937
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + List<? extends Volume> volumes2 = h2.getVolumes();
> + ComparisonChain comparisonChain = ComparisonChain.start().compare(getCores(h1), getCores(h2))
> + .compare(h1.getRam(), h2.getRam())
> + .compare(getSpace(h1), getSpace(h2))
> + .compare(getBootableDeviceType(h1), getBootableDeviceType(h2));
> + if(!volumes1.isEmpty() && !volumes2.isEmpty() && volumes1.size() == volumes2.size()) {
> + for (int i = 0; i < volumes1.size(); i++) {
> + comparisonChain.compare(volumes1.get(i).getType(), volumes2.get(i).getType());
> + }
> + }
> + return comparisonChain.result();
> + }
> + });
> + for (Integer cpus : virtualGuestConfiguration.getCpusOfProcessors()) {
> + for (Integer memory : virtualGuestConfiguration.getMemories()) {
> + for (VirtualGuestBlockDevice blockDevice : virtualGuestConfiguration.getVirtualGuestBlockDevices()) {
As this is only considering bootable devices, can this loop be the outer one? This way we can save some cpu and memory unnecessary iterations.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11137860
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.compute.domain.Hardware;
> +import org.jclouds.softlayer.domain.Datacenter;
> +import org.jclouds.softlayer.domain.OperatingSystem;
> +import org.jclouds.softlayer.domain.VirtualGuest;
> +import org.testng.annotations.Test;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +import static org.testng.Assert.assertTrue;
> +
> +/**
> + * Tests the function that transforms SoftLayer VirtualGuest to generic hardware.
> + *
> + * @author Andrea Turli
> + */
> +@Test(testName = "VirtualGuestToHardwareTest")
Add `groups="unit"`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882834
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> - public GetImageForVirtualGuest(SoftLayerApi api) {
> - this.api = api;
> - }
> -
> - public Image getImage(VirtualGuest guest) {
> - ProductOrder order = null;
> - // 'bad' orders have no start cpu's and cause the order lookup to fail.
> - if (guest.getStartCpus() < 1)
> - return null;
> - try {
> - order = api.getVirtualGuestApi().getOrderTemplate(guest.getId());
> - } catch (SoftLayerOrderItemDuplicateException e) {
> - // this is a workaround because SoftLayer throws sometimes 500 internal server errors for the above method call
> - logger.warn(e, "Cannot get order template for virtualGuestId(%s)", guest.getId());
> + builder.privateAddresses(ImmutableSet.of(from.getPrimaryBackendIpAddress()));
> + if(from.getTagReferences() != null && !from.getTagReferences().isEmpty()) {
[minor] space after the if.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882238
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
Haven't had a chance to review the code yet, unfortunately, but I see there are 15 [Checkstyle warnings](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/733/org.apache.jclouds.provider$softlayer/violations/). Do you think you could have a look at those, @andreaturli? Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-39658000
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + @Nullable
> + public SoftwareDescription getSoftwareDescription() {
> + return this.softwareDescription;
> + }
> +
> + @Override
> + public int hashCode() {
> + return Objects.hashCode(id);
> + }
> +
> + @Override
> + public boolean equals(Object obj) {
> + if (this == obj) return true;
> + if (obj == null || getClass() != obj.getClass()) return false;
> + VirtualDiskImageSoftware that = VirtualDiskImageSoftware.class.cast(obj);
> + return Objects.equal(this.id, that.id);
As opposed to the rest of domain objects, here you're only checking the id. Is there a special reason for this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139078
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1365](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1365/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-46187763
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> + private final int statusId;
> + private final String mountType;
> + private final String mountMode;
> + private final int bootableFlag;
> + private final String device;
> + private final VirtualDiskImage virtualDiskImage;
> + private final VirtualGuest guest;
> +
> + @ConstructorProperties({
> + "id", "uuid", "statusId", "mountType", "mountMode", "bootableFlag", "device", "diskImage", "guest"
> + })
> + protected VirtualGuestBlockDevice(int id, String uuid, int statusId, String mountType, String mountMode,
> + int bootableFlag, String device, VirtualDiskImage virtualDiskImage,
> + VirtualGuest guest) {
> + this.id = id;
> + this.uuid = uuid;
Agreed! I was not saying to remove `@ContructorProperties` :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10467015
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> /**
> * Tests behavior of {@code VirtualGuestApi}
> - *
> - * @author Adrian Cole
> + *
> + * @author Adrian Cole, Andrea Turli
Same author thing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139913
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + else if (description.startsWith(VYATTACE)) return OsFamily.LINUX;
> + }
> + return OsFamily.UNRECOGNIZED;
> + }
> + };
> + }
> +
> + @Override
> + public Image apply(VirtualGuest from) {
> +
> + checkNotNull(from, "from");
> + ImageBuilder builder = new ImageBuilder().ids(from.getId() + "")
> + .name(from.getHostname())
> + .status(Image.Status.AVAILABLE);
> +
> + if(from.getOperatingSystem() == null) {
[minor] space after `if`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11136923
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
Closed #296.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#event-137605769
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> this.hourlyRecurringFee = hourlyRecurringFee;
> + this.recurringFee = recurringFee;
Add null check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138709
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
@andreaturli @ahgittin Sorry for the delay. I've been quite busy but I'll review it tonight!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36610828
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @QueryParams(keys = "objectMask", values = LIST_GUEST_MASK)
> @Consumes(MediaType.APPLICATION_JSON)
> @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> Set<VirtualGuest> listVirtualGuests();
>
> /**
> - *
> + * Enables the creation of computing instances on an account.
> + * @param virtualGuest this data type presents the structure in which all virtual guests will be presented.
> + * @return the new Virtual Guest
> + * @see <a href="http://sldn.softlayer.com/reference/services/SoftLayer_Virtual_Guest/createObject" />
> + */
> + @Named("VirtualGuests:create")
> + @POST
> + @Path("SoftLayer_Virtual_Guest")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Produces(MediaType.APPLICATION_JSON)
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + VirtualGuest createObject(@BinderParam(VirtualGuestToJson.class) VirtualGuest virtualGuest);
Rename to `createVirtualGuest`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452480
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> })
> protected VirtualGuest(int accountId, @Nullable Date createDate, boolean dedicatedAccountHostOnly, @Nullable String domain,
> @Nullable String fullyQualifiedDomainName, @Nullable String hostname, int id, @Nullable Date lastVerifiedDate,
> int maxCpu, @Nullable String maxCpuUnits, int maxMemory, @Nullable Date metricPollDate, @Nullable Date modifyDate,
> @Nullable String notes, boolean privateNetworkOnly, int startCpus, int statusId, @Nullable String uuid,
> @Nullable String primaryBackendIpAddress, @Nullable String primaryIpAddress, @Nullable BillingItem billingItem,
> - @Nullable OperatingSystem operatingSystem, @Nullable Datacenter datacenter, @Nullable PowerState powerState) {
> + @Nullable OperatingSystem operatingSystem, @Nullable String operatingSystemReferenceCode,
> + @Nullable Datacenter datacenter, @Nullable PowerState powerState, @Nullable SoftwareLicense softwareLicense,
> + @Nullable int activeTransactionCount, Set<VirtualGuestBlockDevice> blockDevices,
Remove the `@Nullable` from the primitive types.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139011
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> + String operatingSystemReferenceCode = checkNotNull(virtualGuest.getOperatingSystem()
> + .getOperatingSystemReferenceCode(), "operatingSystemReferenceCode");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true,
> + operatingSystemReferenceCode, null, localDiskFlag, new Datacenter(datacenterName), networkComponents,
> + getBlockDevices(virtualGuest));
> + } else if(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup() != null) {
> + String globalIdentifier = checkNotNull(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup()
> + .getGlobalIdentifier(), "blockDeviceTemplateGroup.globalIdentifier");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true, null,
> + new BlockDeviceTemplateGroup(globalIdentifier), localDiskFlag, new Datacenter(datacenterName),
> + networkComponents, null);
> + }
> + return json.toJson(ImmutableMap.of("parameters", ImmutableList.of(templateObject)));
> + }
> +
> + private Set<BlockDevice> getBlockDevices(VirtualGuest virtualGuest) {
not needed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r14451169
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
@demobox I think this PR needs to be applied first to 1.7.x and to `master` as well. Are you suggesting a different workflow?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36007673
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
Thank you very much for your review, Ignasi!
I think your comments make really sense to me and the summary is quite useful
Sorry for the missing (important!) tests but I'll add them while I address your comments.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-37273813
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> /**
> * Converts an Datacenter into a Location.
> */
> -public class DatacenterToLocation implements Function<Datacenter,Location> {
> +public class DatacenterToLocation implements Function<Datacenter,Location> {
Can this function be `@Singleton`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882073
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
closing this PR as it has been re-opened against master at https://github.com/jclouds/jclouds/pull/431
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-47831666
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #733](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/733/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-39557355
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
Very nice cleanup Andrea. Many thanks for your effort keeping this up to date and for refactoring the code to increase the provider performance! I've been through the entire PR now. Apart from what I've commented, let me summarize what should be changed:
* In the domain classes, for properties whose name does not match the serialized form, add the corresponding `@Named` annotation. That would make serialization and deserialization consistent.
* All constructors of the domain objects should have `null` checks for their mandatory properties.
* Don't use the `ConcreteBuilder + abstract Builder` pattern if the builder is not meant to be extended.
* Rename API methods to something meaningful rather than the generic `getObject()`, etc.
* There are several classes with unused imports and (quite long) blocks of commented code. Could you clean them up?
* Avoid using `System.out`, even in tests.
* **Important**: Add the expect and live tests for the `AccountApi` and `VirtualGuestBlockDeviceTemplateGroupApi` classes.
* **Important**: Add the expect and live tests for the missing methods from the `VirtualGuestApi` class.
Also, it would be great if, once finished addressing the comments, you could paste the result of running the live tests.
And huge thanks for your effort on this refactor! Changing the entire provider is not an easy task, but the new code looks promising.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-37242142
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> */
> @RequestFilters(BasicAuthentication.class)
> @Path("/v{jclouds.api-version}")
> public interface AccountApi {
>
> + public static String GUEST_MASK = "children.blockDevices.diskImage.softwareReferences.softwareDescription";
> + public static String LIST_GUEST_MASK = "powerState;operatingSystem.passwords;datacenter;billingItem;blockDevices" +
> + ".diskImage;tagReferences";
> +
> + /**
> + * @return an account's associated virtual guest objects.
> + */
> + @Named("Account:listVirtualGuest")
> + @GET
> + @Path("/SoftLayer_Account/VirtualGuests")
> + @QueryParams(keys = "objectMask", values = LIST_GUEST_MASK)
> + @Consumes(MediaType.APPLICATION_JSON)
If all methods use the same media types, move the `@Consumes` annotation to class level.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882748
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
Will continue reviewing tomorrow (it is a long one, and it is difficult to stay focused after quit an hour reviewing :)).
One important consideration, though: this PR breaks existing code, as the way it manages the Compute resources (locations, hardware, etc) is incompatible with the previous version. I'm not very keen on merging this kind of breaking changes in minor releases, so I'd rather add the changes in this PR to 1.8 (we can continue the review here, no need to start again).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36684801
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> + *
> + * @author Andrea Turli
> + */
> +@Singleton
> +public class TagToJson implements Binder {
> +
> + private final Json json;
> +
> + @Inject
> + public TagToJson(Json json) {
> + this.json = checkNotNull(json, "json");
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + Set<String> tags = Set.class.cast(checkNotNull(input, "parameters"));
Do we need a `checkArgument` on the input here, or do we know that it will be a set? And should that be `checkNotNull(input, "input")`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324378
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> + private VirtualGuest createVirtualGuest() {
> + return VirtualGuest.builder()
> + .domain("example.com")
> + .hostname("host1")
> + .id(1301396)
> + .maxMemory(1024)
> + .startCpus(1)
> + .operatingSystem(OperatingSystem.builder().id("UBUNTU_LATEST")
> + .operatingSystemReferenceCode("UBUNTU_LATEST")
> + .build())
> + .datacenter(Datacenter.builder().name("test").build())
> + .build();
> }
> }
There are many expect tests missing here. Add the tests for ALL the methods in the api class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139874
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + logger.debug("Cannot determine os version for item: %s", operatingSystem);
> + }
> + if (bits == null) {
> + logger.debug("Cannot determine os bits for item: %s", operatingSystem);
> + }
> +
> + org.jclouds.compute.domain.OperatingSystem os = org.jclouds.compute.domain.OperatingSystem.builder()
> + .description(optLongDescription.isPresent() ? optLongDescription.get() : UNRECOGNIZED)
> + .family(osFamily)
> + .version(osVersion)
> + .is64Bit(Objects.equal(bits, 64))
> + .build();
> +
> + return new ImageBuilder()
> + .ids(operatingSystem.getId())
> + .description(optOSReferenceCode.isPresent() ? optOSReferenceCode.get() : UNRECOGNIZED)
Use just `optOSReferenceCode.or(UNRECOGNIZED)`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882111
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> @@ -47,17 +47,17 @@ public DatacenterToLocation(JustProvider provider) {
>
> @Override
> public Location apply(Datacenter datacenter) {
> - return new LocationBuilder().scope(LocationScope.ZONE)
> - .metadata(ImmutableMap.<String, Object>of())
> + return new LocationBuilder().id(datacenter.getName())
agreed. I will re-open it against master
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r14450984
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> this.guestLoginDelay = guestLoginDelay;
> - this.productPackageSupplier = checkNotNull(productPackageSupplier, "productPackageSupplier");
> + this.activeTransactionsDelay = activeTransactionsDelay;
> + this.includePublicImages = includePublicImages;
Add null checks for all properties.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882275
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.rest.annotations.Fallback;
> +import org.jclouds.rest.annotations.QueryParams;
> +import org.jclouds.rest.annotations.RequestFilters;
> +import org.jclouds.softlayer.domain.ProvisioningVersion1Transaction;
> +import org.jclouds.softlayer.domain.VirtualGuestBlockDeviceTemplateGroup;
> +
> +import javax.inject.Named;
> +import javax.ws.rs.Consumes;
> +import javax.ws.rs.GET;
> +import javax.ws.rs.Path;
> +import javax.ws.rs.PathParam;
> +import javax.ws.rs.core.MediaType;
> +import java.util.Set;
> +
> +/**
> + * Provides asynchronous access to VirtualGuestBlockDeviceTemplateGroup via their REST API.
Remove "asynchronous" from the javadoc.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452639
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1110](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1110/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36767891
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + private final int statusId;
> + private final String mountType;
> + private final String mountMode;
> + private final int bootableFlag;
> + private final String device;
> + private final VirtualDiskImage virtualDiskImage;
> + private final VirtualGuest guest;
> +
> + @ConstructorProperties({
> + "id", "uuid", "statusId", "mountType", "mountMode", "bootableFlag", "device", "diskImage", "guest"
> + })
> + protected VirtualGuestBlockDevice(int id, String uuid, int statusId, String mountType, String mountMode,
> + int bootableFlag, String device, VirtualDiskImage virtualDiskImage,
> + VirtualGuest guest) {
> + this.id = id;
> + this.uuid = uuid;
The `@ConstructorProperties`, though, needs to have all parameters, as jclouds uses it to properly pass the parameters to the constructor (order is important there!).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10466941
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1116](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1116/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-37188981
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -236,28 +414,34 @@ public void destroyNode(String id) {
> VirtualGuest guest = getNode(id);
> if (guest == null)
> return;
> -
> - if (guest.getBillingItemId() == -1)
> - throw new IllegalStateException(String.format("no billing item for guest(%s) so we cannot cancel the order",
> - id));
> -
> - logger.debug(">> canceling service for guest(%s) billingItem(%s)", id, guest.getBillingItemId());
> - client.getVirtualGuestApi().cancelService(guest.getBillingItemId());
> + logger.debug(">> awaiting virtualGuest(%s) without active transactions", guest.getId());
> + checkState(retry(new Predicate<VirtualGuest>() {
> + public boolean apply(VirtualGuest guest) {
> + guest = getNode(guest.getId() + "");
Can you do the same without re-assigning the variable passed as a parameter?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138223
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> }
>
> @Override
> - public Iterable<ProductItem> listImages() {
> - return filter(productPackageSupplier.get().getItems(), categoryCode("os"));
> + public Set<OperatingSystem> listImages() {
> + Set<OperatingSystem> result = Sets.newHashSet();
> + Set<SoftwareDescription> unfiltered = api.getSoftwareDescriptionApi().getAllObjects();
> + Set<OperatingSystem> osAvailable = createObjectOptionsSupplier.get().getVirtualGuestOperatingSystems();
> + for (OperatingSystem os : osAvailable) {
> + final String operatingSystemReferenceCode = os.getOperatingSystemReferenceCode();
> + result.addAll(
> + FluentIterable.from(unfiltered)
> + .filter(new Predicate<SoftwareDescription>() {
> + @Override
> + public boolean apply(SoftwareDescription input) {
> + return input.getOperatingSystem() == 1 && input.getReferenceCode().equals
Replace the `1` with a constant that describes what actually that value is?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276417
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> }
>
> @Override
> public String toString() {
> - return string().toString();
> + return "OperatingSystem{" +
> + "id='" + id + '\'' +
Stupid IDE :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11151050
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + this.id = id;
> + this.softwareDescription = softwareDescription;
> + this.softwareDescriptionId = softwareDescriptionId;
> + }
> +
> + public int getId() {
> + return this.id;
> + }
> +
> + @Nullable
> + public SoftwareDescription getSoftwareDescription() {
> + return this.softwareDescription;
> + }
> +
> + /**
> + * @return A longer location description.
Incorrect javadoc comment.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138845
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> @@ -107,13 +176,30 @@ protected ConcreteBuilder self() {
> private final String country;
> private final String state;
> private final String description;
> + private final int accountId;
> + private final String address;
good spot, thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11151003
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -21,7 +21,7 @@
> import org.testng.annotations.Test;
>
> /**
> - * Tests behavior of {@code SoftLayerApi}
> + * Tests behavior of {@code SoftLayerClient}
It was ok as `SoftLayerApi`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139686
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> + .getOperatingSystemReferenceCode(), "operatingSystemReferenceCode");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true,
> + operatingSystemReferenceCode, null, localDiskFlag, new Datacenter(datacenterName), networkComponents,
> + getBlockDevices(virtualGuest));
> + } else if(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup() != null) {
> + String globalIdentifier = checkNotNull(virtualGuest.getVirtualGuestBlockDeviceTemplateGroup()
> + .getGlobalIdentifier(), "blockDeviceTemplateGroup.globalIdentifier");
> + templateObject = new TemplateObject(hostname, domain, startCpus, maxMemory, true, null,
> + new BlockDeviceTemplateGroup(globalIdentifier), localDiskFlag, new Datacenter(datacenterName),
> + networkComponents, null);
> + }
> + return json.toJson(ImmutableMap.of("parameters", ImmutableList.of(templateObject)));
> + }
> +
> + private Set<BlockDevice> getBlockDevices(VirtualGuest virtualGuest) {
> + if (virtualGuest.getVirtualGuestBlockDevices() == null) return null;
[minor] Write as
```
if (virtualGuest.getVirtualGuestBlockDevices() == null) {
return null;
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324394
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + private final String upgradeSoftwareDescriptionId;
> + private final String upgradeSwDescId;
> + private final String virtualLicense;
> + private final String virtualizationPlatform;
> +
> + @ConstructorProperties({
> + "id", "longDescription", "manufacturer", "name", "operatingSystem", "referenceCode", "requiredUser",
> + "version", "controlPanel", "upgradeSoftwareDescriptionId", "upgradeSwDescId", "virtualLicense",
> + "virtualizationPlatform"
> + })
> + protected SoftwareDescription(int id, @Nullable String longDescription, @Nullable String manufacturer,
> + @Nullable String name, int operatingSystem, @Nullable String referenceCode,
> + @Nullable String requiredUser, @Nullable String version, int controlPanel,
> + @Nullable String upgradeSoftwareDescriptionId, @Nullable String upgradeSwDescId,
> + @Nullable String virtualLicense, @Nullable String virtualizationPlatform) {
> + this.id = checkNotNull(id, "id");
No need to check for null in primitives.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882646
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + private final String name;
> + private final int port;
> + private final int speed;
> + private final String status;
> +
> + @ConstructorProperties({
> + "id", "uuid", "guestId", "networkId", "macAddress", "maxSpeed", "name", "port", "speed",
> + "status"
> + })
> + protected VirtualGuestNetworkComponent(int id, String uuid, int guestId, int networkId, @Nullable String macAddress,
> + int maxSpeed, @Nullable String name, int port, int speed,
> + @Nullable String status) {
> + this.id = checkNotNull(id, "id");
> + this.uuid = uuid;
> + this.guestId = checkNotNull(guestId, "guestId");
> + this.networkId = checkNotNull(networkId, "networkId");
Same thing about primitives.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139207
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -128,4 +135,11 @@ protected void tearDown() {
> protected Iterable<Module> setupModules() {
> return ImmutableSet.<Module> of(getLoggingModule(), new SshjSshClientModule());
> }
> +
> + @Override
> + protected Properties setupProperties() {
> + Properties properties = super.setupProperties();
> + properties.setProperty("jclouds.ssh.max-retries", "10");
> + return properties;
> + }
Just seen the BaseComputeServiceLiveTest is implemented in a different class :) Great!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452971
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> +import com.google.common.base.Splitter;
> +import com.google.common.collect.Iterables;
> +
> +import static com.google.common.collect.Iterables.getLast;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class OperatingSystems {
> +
> + public static Function<String, Integer> bits() {
> + return new Function<String, Integer>() {
> + @Override
> + public Integer apply(String operatingSystemReferenceCode) {
> + if (operatingSystemReferenceCode != null) {
> + return Integer.parseInt(getLast(Splitter.on("_").split(operatingSystemReferenceCode)));
Great! I've learned something new, thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11150742
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1185](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1185/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-38793598
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #884](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/884/) 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/pull/296#issuecomment-36623644
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> .build();
> }
>
> private Iterable<String> createIso3166Codes(Address address) {
> - if (address == null) return ImmutableSet.<String> of();
> + if (address== null) return ImmutableSet.<String> of();
[minor] Add the space back again
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11136645
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.softlayer.domain.VirtualGuest;
> +import org.jclouds.softlayer.domain.VirtualGuestBlockDevice;
> +
> +import javax.inject.Inject;
> +import java.util.HashSet;
> +import java.util.Set;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * Converts a VirtualGuest into a json string valid for creating a CCI via softlayer api
> + * The string is set into the payload of the HttpRequest
> + *
> + * @author Andrea Turli
> + */
> +public class VirtualGuestToJson implements Binder {
Thanks for the explanation. Let's keep it as-is then.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10299403
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds #854 UNSTABLE
Looks like an unrelated [test failure](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/org.apache.jclouds.api$openstack-nova/854/testReport/junit/org.jclouds.openstack.nova.v2_0.compute/NovaComputeServiceExpectTest/testCreateNodeWhileUserSpecifiesKeyPair/)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-35620817
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by ahgittin <no...@github.com>.
Awesome job @andreaturli . I've been using this -- demoed at IBM Pulse -- and it works great, fixes a bunch of issues present in 1.7.1. Look forward to having this merged so I don't have to maintain a branch!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36606781
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import com.google.common.base.Objects;
> +
> +import java.beans.ConstructorProperties;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class TagType {
> + private final String keyName;
> + private final String description;
> +
> + @ConstructorProperties({"keyName", "description"} )
> + public TagType(String keyName, String description) {
> + this.keyName = keyName;
> + this.description = description;
Add null checks if the parameters are mandatory.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882679
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + * @see <a href= "http://sldn.softlayer.com/reference/datatypes/SoftLayer_Container_Virtual_Guest_Configuration"/
> + * *
> + */
> +public class ContainerVirtualGuestConfiguration {
> +
> + public static final String SWAP_DEVICE = "1";
> +
> + public static Builder<?> builder() {
> + return new ConcreteBuilder();
> + }
> +
> + public Builder<?> toBuilder() {
> + return new ConcreteBuilder().fromContainerVirtualGuestConfiguration(this);
> + }
> +
> + public abstract static class Builder<T extends Builder<T>> {
If this builder is not going to be subclassed, don't make it an abstract class and remove the `ConcreteBuilder`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882559
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + private final int groupId;
> + private final String units;
> + private final VirtualDiskImage diskImage;
> +
> + @ConstructorProperties({
> + "id", "device", "diskImageId", "diskSpace", "groupId", "units", "diskImage"
> + })
> + protected VirtualGuestBlockDeviceTemplate(int id, String device, int diskImageId, float diskSpace, int groupId,
> + String units, VirtualDiskImage diskImage) {
> + this.id = id;
> + this.device = device;
> + this.diskImageId = diskImageId;
> + this.diskSpace = diskSpace;
> + this.groupId = groupId;
> + this.units = units;
> + this.diskImage = diskImage;
Same again. Add null checks for all mandatory properties or add the corresponding `@Nullable` annotations.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139129
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #635](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/635/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36624550
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + @ConstructorProperties({
> + "id", "uuid", "capacity", "units", "typeId", "description", "name", "storageRepositoryId", "softwareReferences"
> + })
> + public VirtualDiskImage(int id, String uuid, float capacity, String units, int typeId, String description,
> + String name, int storageRepositoryId, @Nullable Set<VirtualDiskImageSoftware> softwareReferences) {
> + this.id = id;
> + this.uuid = uuid;
> + this.capacity = capacity;
> + this.units = units;
> + this.typeId = typeId;
> + this.description = description;
> + this.name = name;
> + this.storageRepositoryId = storageRepositoryId;
> + this.softwareReferences = softwareReferences == null ? ImmutableSet.<VirtualDiskImageSoftware>of() :
> + ImmutableSet.copyOf(softwareReferences);
> +
Same again, add null checks for all mandatory properties.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138909
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> */
> public class SoftLayerParserModule extends AbstractModule {
>
> @Override
> protected void configure() {
> - bind(DateAdapter.class).to(Iso8601DateAdapter.class);
> + bind(GsonModule.DateAdapter.class).to(GsonModule.Iso8601DateAdapter.class);
Any reason for the import change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138278
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> - @ConstructorProperties({
> - "id", "itemId", "recurringFee", "hourlyRecurringFee", "item", "categories"
> - })
> - protected ProductItemPrice(int id, long itemId, @Nullable Float recurringFee, @Nullable Float hourlyRecurringFee, @Nullable ProductItem item, @Nullable Set<ProductItemCategory> categories) {
> +
> + public ProductItemPrice(int id, int hourlyRecurringFee, String recurringFee, ProductItem item) {
Missing the `@ConstructorProperties`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138720
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> @Override
> - public boolean apply(ProductItem input) {
> - return ProductItemToImage.imageId().apply(input).equals(id);
> + public boolean apply(OperatingSystem input) {
> + return input.equals(id);
`OperatingSystem.equals(String)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276474
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> @ConstructorProperties({
> - "accountId", "createDate", "dedicatedAccountHostOnlyFlag", "domain", "fullyQualifiedDomainName", "hostname", "id", "lastVerifiedDate", "maxCpu", "maxCpuUnits", "maxMemory", "metricPollDate", "modifyDate", "notes", "privateNetworkOnlyFlag", "startCpus", "statusId", "uuid", "primaryBackendIpAddress", "primaryIpAddress", "billingItem", "operatingSystem", "datacenter", "powerState"
> + "accountId", "createDate", "dedicatedAccountHostOnlyFlag", "domain", "fullyQualifiedDomainName", "hostname",
Make sure that all fields which name is different than the serialized form (such as the `dedicatedAccountHostOnly` one) also have the corresponding `@Named` annotation in the field itself.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10277095
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> + });
> + return set;
> + }
> +
> + private HashSet<NetworkComponent> getNetworkComponents(VirtualGuest virtualGuest) {
> + if(virtualGuest.getVirtualGuestNetworkComponents() == null) return null;
> + return Sets.newHashSet(Iterables.transform(virtualGuest.getVirtualGuestNetworkComponents(),
> + new Function<VirtualGuestNetworkComponent, NetworkComponent>() {
> + @Override
> + public NetworkComponent apply(VirtualGuestNetworkComponent input) {
> + return new NetworkComponent(input.getSpeed());
> + }
> + }));
> + }
> +
> + private static class TemplateObject {
Why are all these classes here? Are we creating something like a "parallel domain model" here? I saw the explanation about the Binder above, but if this is really what the SoftLayer API expects on the Create call, shouldn't these be domain objects, then?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324413
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
There are still some classes that perform quite an amount of logic that are missing unit tests:
* VirtualGuestToJson
* In the `compute.functions` package, there are only unit tests for the `DatacenterToLocation` function. All other classes in that package should have the corresponding unit tests too. There are several combinations in each class, so it is important to have them properly unit tested.
Can you address the comments (most are minor) and add the missing tests (tests are important) @andreaturli ?
Thanks again for your hard work on this!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-39150023
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> this.id = id;
> this.description = description;
> - this.units = units;
> - this.capacity = capacity;
> - this.prices = prices == null ? ImmutableSet.<ProductItemPrice>of() : ImmutableSet.copyOf(prices);
> - this.categories = categories == null ? ImmutableSet.<ProductItemCategory>of() : ImmutableSet.copyOf(categories);
> + this.softwareDescriptionId = softwareDescriptionId;
> + this.softwareDescription = softwareDescription;
Add null checks for all non-nullable properties.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138662
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> + @Override
> + public BlockDevice apply(VirtualGuestBlockDevice input) {
> + return new BlockDevice(input.getDevice(), input.getVirtualDiskImage().getCapacity());
> + }
> + })
> + .toSortedSet(new Comparator<BlockDevice>() {
> + @Override
> + public int compare(BlockDevice b1, BlockDevice b2) {
> + return Integer.valueOf(b1.getDevice()).compareTo(Integer.valueOf(b2.getDevice()));
> + }
> + });
> + return set;
> + }
> +
> + private HashSet<NetworkComponent> getNetworkComponents(VirtualGuest virtualGuest) {
> + if(virtualGuest.getVirtualGuestNetworkComponents() == null) return null;
See comment above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324407
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -197,20 +181,4 @@ public SoftLayerTemplateOptions userMetadata(Map<String, String> userMetadata) {
> public SoftLayerTemplateOptions userMetadata(String key, String value) {
> return SoftLayerTemplateOptions.class.cast(super.userMetadata(key, value));
> }
> -
> - /**
> - * {@inheritDoc}
> - */
> - @Override
> - public SoftLayerTemplateOptions nodeNames(Iterable<String> nodeNames) {
> - return SoftLayerTemplateOptions.class.cast(super.nodeNames(nodeNames));
> - }
Why has this been removed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276104
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + * Class VirtualGuestBlockDeviceTemplate
> + *
> + * @author Andrea Turli
> + * @see <a href= "http://sldn.softlayer.com/reference/datatypes/SoftLayer_Virtual_Guest_Block_Device_Template"/>
> + */
> +public class VirtualGuestBlockDeviceTemplate {
> +
> + public static Builder<?> builder() {
> + return new ConcreteBuilder();
> + }
> +
> + public Builder<?> toBuilder() {
> + return new ConcreteBuilder().fromVirtualGuestBlockDeviceTemplate(this);
> + }
> +
> + public abstract static class Builder<T extends Builder<T>> {
Same comment about builders. Don't make it abstract and remove the `ConcreteBuilder`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882722
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * Converts a VirtualGuest into a json string valid for creating a CCI via softlayer api
> + * The string is set into the payload of the HttpRequest
> + *
> + * @author Andrea Turli
> + */
> +public class VirtualGuestToJson implements Binder {
> +
> + private Json json;
> +
> + @Inject
> + public VirtualGuestToJson(Json json) {
> + this.json = json;
Add a `checkNotNull` check
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10014103
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> * @see <a href="http://sldn.softlayer.com/article/REST" />
> - * @author Adrian Cole
> + * @author Adrian Cole, Andrea Turli
Add each author in a different line
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139263
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @Consumes(MediaType.APPLICATION_JSON)
> - @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> - ProductOrder getOrderTemplate(@PathParam("id") long id);
> -
> + @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
This should be `NullOnNotFoundOr404`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139440
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> /**
> *
> - * @author Jason King
> + * @author Jason King, Andrea Turli
Same again, each author in a separate line.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139634
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> */
> @Delegate
> - ProductPackageApi getProductPackageApi();
Are we totally removing this? If so, is that a change to add to a point release such as 1.7.2?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324375
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.collect.FluentIterable;
> +import com.google.common.collect.ImmutableList;
> +import com.google.common.collect.Iterables;
> +import org.jclouds.compute.domain.Hardware;
> +import org.jclouds.compute.domain.HardwareBuilder;
> +import org.jclouds.compute.domain.Processor;
> +import org.jclouds.compute.domain.Volume;
> +import org.jclouds.compute.domain.internal.VolumeImpl;
> +import org.jclouds.softlayer.domain.VirtualGuest;
> +import org.jclouds.softlayer.domain.VirtualGuestBlockDevice;
> +
> +import javax.inject.Singleton;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import static com.google.common.collect.FluentIterable.from;
> +import static com.google.common.collect.Iterables.filter;
Remove unused imports
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10275959
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.compute.domain.OperatingSystem;
> +import org.jclouds.compute.domain.OsFamily;
> +import org.jclouds.compute.domain.Processor;
> +import org.jclouds.compute.reference.ComputeServiceConstants;
> +import org.jclouds.logging.Logger;
> +import org.jclouds.softlayer.domain.SoftwareLicense;
> +import org.jclouds.softlayer.domain.VirtualGuest;
> +
> +import javax.annotation.Resource;
> +import javax.inject.Named;
> +import javax.inject.Singleton;
> +
> +import java.util.regex.Matcher;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import static com.google.common.collect.Iterables.getLast;
Same here, remove unused imports.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10275991
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1080](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1080/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-35519785
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.base.Objects;
> +import com.google.common.base.Predicate;
> +import com.google.common.collect.FluentIterable;
> +import com.google.common.collect.ImmutableSet;
> +import com.google.common.collect.Iterables;
> +import com.google.common.collect.Sets;
> +
> +import java.beans.ConstructorProperties;
> +import java.util.Set;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * @author Andrea Turli
> + * @see <a href= "http://sldn.softlayer.com/reference/datatypes/SoftLayer_Container_Virtual_Guest_Configuration"/
> + * *
[minor] Remove the `*`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882478
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @Consumes(MediaType.APPLICATION_JSON)
> @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
> - void powerOnVirtualGuest(@PathParam("id") long id);
> + Void rebootHardVirtualGuest(@PathParam("id") long id);
Why returning an object and not the `void` primitive?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452594
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import javax.inject.Inject;
> +import java.util.HashSet;
> +import java.util.Set;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * Converts a VirtualGuest into a json string valid for creating a CCI via softlayer api
> + * The string is set into the payload of the HttpRequest
> + *
> + * @author Andrea Turli
> + */
> +public class VirtualGuestToJson implements Binder {
> +
> + private Json json;
Make this final
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10014087
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> })
> - protected Address(int id, String country, @Nullable String state, @Nullable String description) {
> - this.id = id;
> - this.country = checkNotNull(emptyToNull(country), "country cannot be null or empty:" + country);
> + protected Address(int id, String country, @Nullable String state, @Nullable String description, @Nullable int accountId,
> + @Nullable String address, @Nullable String city, @Nullable String contactName,
> + @Nullable int isActive, @Nullable int locationId, @Nullable String postalCode) {
Primitive types can't be `null`; don't mark them as `@Nullable`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138368
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + /*
> + for(ComputeMetadata node : context.getComputeService().listNodes()) {
> + logger.info("node: ", node);
> + }
> +
> + for(Hardware hardware : context.getComputeService().listHardwareProfiles()) {
> + logger.info("hardware: ", hardware);
> + }
> +
> + for(Image image : context.getComputeService().listImages()) {
> + logger.info("image: ", image);
> + }
> +
> + Image image = context.getComputeService().getImage("UBUNTU_8_64");
> + logger.info("UBUNTU_8_64 image: ", image);
> + */
Can all the commented code in this class be removed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139571
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @Provides
> @Singleton
> @Memoized
> - public Supplier<ProductPackage> getProductPackage(AtomicReference<AuthorizationException> authException,
> - @Named(PROPERTY_SESSION_INTERVAL) long seconds, final SoftLayerApi client,
> - @Named(PROPERTY_SOFTLAYER_VIRTUALGUEST_PACKAGE_NAME) final String virtualGuestPackageName) {
> + public Supplier<ContainerVirtualGuestConfiguration> getCreateObjectOptions(AtomicReference<AuthorizationException> authException,
> + @Named(PROPERTY_SESSION_INTERVAL) long
> + seconds, final SoftLayerApi api) {
Weird formatting here? Put the variable and the type in the same line.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10275297
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @Consumes(MediaType.APPLICATION_JSON)
> - @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> - ProductPackage getProductPackage(@PathParam("id") long id);
> + @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> + Set<SoftwareDescription> getAllObjects();
I'd rather use the `listSoftwareDescriptions` one, but it is a matter of taste. Let's see if anyone has a different opinion. @demobox?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10468218
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> +
> + private Set<BlockDevice> getBlockDevices(VirtualGuest virtualGuest) {
> + if (virtualGuest.getVirtualGuestBlockDevices() == null) return null;
> + Set<BlockDevice> set = FluentIterable.from(virtualGuest.getVirtualGuestBlockDevices())
> + .transform(new Function<VirtualGuestBlockDevice, BlockDevice>() {
> + @Override
> + public BlockDevice apply(VirtualGuestBlockDevice input) {
> + return new BlockDevice(input.getDevice(), input.getVirtualDiskImage().getCapacity());
> + }
> + })
> + .toSortedSet(new Comparator<BlockDevice>() {
> + @Override
> + public int compare(BlockDevice b1, BlockDevice b2) {
> + return Integer.valueOf(b1.getDevice()).compareTo(Integer.valueOf(b2.getDevice()));
> + }
> + });
Unless there's a really good reason to use Guava here, just write as:
```
ImmutableSortedSet.Builder<BlockDevice> blockDevices = ImmutableSortedSet.orderedBy(new Comparator...);
for (VirtualGuestBlockDevice blockDevice : virtualGuest.getVirtualGuestBlockDevices()) {
blockDevices.add(new BlockDevice(input.getDevice(), input.getVirtualDiskImage().getCapacity());
}
return blockDevices.build();
```
It's just so much easier to read and understand ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324404
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + public T activeTransactionCount(int activeTransactionCount) {
> + this.activeTransactionCount = activeTransactionCount;
> + return self();
> + }
> +
> + /**
> + * @see VirtualGuest#getVirtualGuestBlockDevices()
> + */
> +
> + public T blockDevices(Set<VirtualGuestBlockDevice> blockDevices) {
> + this.blockDevices = ImmutableSet.copyOf(checkNotNull(blockDevices, "blockDevices"));
> + return self();
> + }
> +
> + public T blockDevices(VirtualGuestBlockDevice... in) {
> + return blockDevices(ImmutableSet.copyOf(in));
Add a check to prevent a `null` input.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10451752
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #657](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/657/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-37484045
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #954](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/954/) UNSTABLE
Looks like there's a problem with this pull request
[(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/pull/296#issuecomment-38706561
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> }
>
> @Override
> public String toString() {
> - return string().toString();
> + return "OperatingSystem{" +
> + "id='" + id + '\'' +
[minor] Why single-quoting the id?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138619
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> <version>${project.version}</version>
> <scope>test</scope>
> </dependency>
> <dependency>
> - <groupId>org.apache.jclouds.driver</groupId>
> - <artifactId>jclouds-sshj</artifactId>
> - <version>${project.version}</version>
> - <scope>test</scope>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-slf4j</artifactId>
> + <version>${project.version}</version>
> + <scope>test</scope>
> + </dependency>
> + <dependency>
> + <groupId>ch.qos.logback</groupId>
> + <artifactId>logback-classic</artifactId>
> + <scope>test</scope>
[minor] Wrong indent here
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324372
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + public SoftLayerComputeServiceContextLiveTest() {
> + provider = "softlayer";
> + }
> +
> + @Test
> + public void testLaunchClusterWithMinDisk() throws RunNodesException {
> + int numNodes = 1;
> + final String name = "node";
> +
> + ComputeServiceContext context = ContextBuilder.newBuilder("softlayer").credentials(identity, credential)
> + .modules(ImmutableSet.of(new SLF4JLoggingModule(),
> + new SshjSshClientModule()))
> + .build(ComputeServiceContext.class);
> +
> + for(Image image : context.getComputeService().listImages()) {
> + System.out.println(image);
Even in tests, use the loggers. Jclouds has a TestNG output listener and no other pieces should directly print to the standard output.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452921
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + /**
> + * Builds a Json string suitable for sending to the softlayer api
> + *
> + * @param virtualGuest
> + * @return String
> + */
> + String buildJson(VirtualGuest virtualGuest) {
> + TemplateObject templateObject = null;
> + String hostname = checkNotNull(virtualGuest.getHostname(), "hostname");
> + String domain = checkNotNull(virtualGuest.getDomain(), "domain");
> + int startCpus = checkNotNull(virtualGuest.getStartCpus(), "startCpus");
> + int maxMemory = checkNotNull(virtualGuest.getMaxMemory(), "maxMemory");
> + boolean localDiskFlag = checkNotNull(virtualGuest.isLocalDiskFlag(), "localDiskFlag");
> + String datacenterName = checkNotNull(virtualGuest.getDatacenter().getName(), "datacenterName");
> + Set<NetworkComponent> networkComponents = getNetworkComponents(virtualGuest);
> + if(virtualGuest.getOperatingSystem() != null) {
[minor] space after `if`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11136395
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> - checkState(orderInSystem, "order for guest %s doesn't have login details within %sms", result,
> - Long.toString(guestLoginDelay));
> - result = client.getVirtualGuestApi().getVirtualGuest(result.getId());
> + private Set<TagReference> getTagReferences(Set<String> tags) {
This method is never used. Remove it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882328
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
@demobox ok thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-36009641
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public T children(VirtualGuestBlockDeviceTemplateGroup... in) {
> + return children(ImmutableSet.copyOf(in));
> + }
> +
> + /**
> + * @see org.jclouds.softlayer.domain.VirtualGuestBlockDeviceTemplateGroup#getBlockDevices()
> + */
> + public T blockDevices(Set<VirtualGuestBlockDeviceTemplate> blockDevices) {
> + this.blockDevices = ImmutableSet.copyOf(checkNotNull(blockDevices, "blockDevices"));
> + return self();
> + }
> +
> + public T blockDevices(VirtualGuestBlockDeviceTemplate... in) {
> + return blockDevices(ImmutableSet.copyOf(in));
Null check here too
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452274
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + public Set<OperatingSystem> getVirtualGuestOperatingSystems() {
> + if(operatingSystems.isEmpty()) return ImmutableSet.of();
> + return Sets.newHashSet(Iterables.transform(operatingSystems,
> + new Function<ContainerVirtualGuestConfigurationOption, OperatingSystem>() {
> + @Override
> + public OperatingSystem apply(ContainerVirtualGuestConfigurationOption input) {
> + return OperatingSystem.builder()
> + .id(input.getTemplate().getOperatingSystemReferenceCode())
> + .operatingSystemReferenceCode(input.getTemplate().getOperatingSystemReferenceCode())
> + .build();
> + }
> + }));
> + }
> +
> + public Set<VirtualGuestBlockDevice> getVirtualGuestBlockDevices() {
> + if(blockDevices.isEmpty()) return ImmutableSet.of();
Same as above.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882545
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @Consumes(MediaType.APPLICATION_JSON)
> - @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> - ProductPackage getProductPackage(@PathParam("id") long id);
> + @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> + Set<SoftwareDescription> getAllObjects();
Most APIs follow the naming convention to use the `list` prefix for methods that return lists. Rename this method to `listSoftwareDescriptions`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452466
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> */
> @Test(groups = "live")
> public class AccountApiLiveTest extends BaseSoftLayerApiLiveTest {
>
> @Test
> - public void testGetActivePackages() {
> - Set<ProductPackage> response = api.getAccountApi().getActivePackages();
> - assert null != response;
> + public void testGetBlockDeviceTemplateGroups() {
> + Set<VirtualGuestBlockDeviceTemplateGroup> privateImages = api().getBlockDeviceTemplateGroups();
> + assertNotNull(privateImages);
Should also be some assertion to check the "contents" of the returned set?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882870
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @Consumes(MediaType.APPLICATION_JSON)
> - @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> - ProductOrder getOrderTemplate(@PathParam("id") long id);
> -
> + @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
> + ProvisioningVersion1Transaction createArchiveTransaction(@PathParam("id") String id,
> + @PathParam("blockDevices") Set<VirtualGuestBlockDevice> blockDevices);
There is no `blockDevices ` path parameter.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139405
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> private final int id;
> private final String description;
> - private final String units;
> - private final Float capacity;
> - private final Set<ProductItemPrice> prices;
> - private final Set<ProductItemCategory> categories;
> + private final String SoftwareDescriptionId;
Start variable name in lower case.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276937
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + return input.getMountType().equals("Disk");
> + }
> + })
> + .transform(new Function<VirtualGuestBlockDevice, Volume>() {
> + @Override
> + public Volume apply(VirtualGuestBlockDevice item) {
> + float volumeSize = item.getVirtualDiskImage().getCapacity();
> + return new VolumeImpl(
> + item.getId() + "",
> + from.isLocalDiskFlag() ? Volume.Type.LOCAL : Volume.Type.SAN,
> + volumeSize, null, item.getBootableFlag() == 1, false);
> + }
> + }).toSet());
> + }
> + return builder.build();
> + }
Reformat this class to 3 space indentation and 120 line length.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11136807
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1169](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1169/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-38707691
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> + });
> + return set;
> + }
> +
> + private HashSet<NetworkComponent> getNetworkComponents(VirtualGuest virtualGuest) {
> + if(virtualGuest.getVirtualGuestNetworkComponents() == null) return null;
> + return Sets.newHashSet(Iterables.transform(virtualGuest.getVirtualGuestNetworkComponents(),
> + new Function<VirtualGuestNetworkComponent, NetworkComponent>() {
> + @Override
> + public NetworkComponent apply(VirtualGuestNetworkComponent input) {
> + return new NetworkComponent(input.getSpeed());
> + }
> + }));
> + }
> +
> + private static class TemplateObject {
Not sure about that, they are used only inside `VirtualGuestToJson` so I think define then as internal classes make sense here. wdyt?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r13794698
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + }));
> + }
> +
> + public Set<Integer> getMemories() {
> + if(memory.isEmpty()) return ImmutableSet.of();
> + return Sets.newHashSet(Iterables.transform(memory, new Function<ContainerVirtualGuestConfigurationOption,
> + Integer>() {
> + @Override
> + public Integer apply(ContainerVirtualGuestConfigurationOption input) {
> + return input.getTemplate().getMaxMemory();
> + }
> + }));
> + }
> +
> + public Set<Datacenter> getVirtualGuestDatacenters() {
> + if(datacenters.isEmpty()) return ImmutableSet.of();
Same as above.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r12882537
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
Hi @andreaturli. Many thanks for this PR! It is a long one and there is much work behind it. Thanks!
I've done a last review, and I think there are no major things to change. The following (and I hope the last steps) should be:
- **General**
- [ ] Check all existing review comments (the ones I've added and the ones that were already there from previous reviews) and address or comment on them.
- **Domain model**
- [ ] Remove the null checks for primitive parameters
- [ ] Remove the abstract/concrete builder pattern and use just a Builder object.
- **Api classes**
- [ ] Move all `@Consumes` annotations at class level.
- **UnitTests**
- [ ] Add tests for the `TagToJson` binder.
- [ ] Add tests for the `OperatingSystems` class.
- [ ] Remove all `System.out` in the `SoftLayerComputeServiceContextLiveTest` class.
There are many tests in this provider, and that is very much appreciated. Thanks for your hard work on this!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-43719145
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrew Phillips <no...@github.com>.
> + * The string is set into the payload of the HttpRequest
> + *
> + * @author Andrea Turli
> + */
> +public class VirtualGuestToJson implements Binder {
> +
> + private final Json json;
> +
> + @Inject
> + public VirtualGuestToJson(Json json) {
> + this.json = checkNotNull(json, "json");
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + VirtualGuest virtualGuest = VirtualGuest.class.cast(checkNotNull(input, "parameters"));
See comments above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11324381
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> + };
> + }
> +
> + private static String parseVersion(String version) {
> + if(version.contains("-")) {
> + String rawVersion = version.substring(0,
> + version.lastIndexOf("-"));
> + if(Iterables.size(Splitter.on(".").split(rawVersion)) == 3) {
> + return rawVersion.substring(0, rawVersion.lastIndexOf("."));
> + } else {
> + return rawVersion;
> + }
> + } else if(version.contains(" ")) {
> + return version.substring(0,
> + version.indexOf(" "));
[minor] Spaces after each `if` and don't break lines here (also in the above call to substring).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11137092
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + * Class VirtualGuestBlockDeviceTemplateGroup
> + *
> + * @author Andrea Turli
> + * @see <a href= "http://sldn.softlayer.com/reference/datatypes/SoftLayer_Virtual_Guest_Block_Device_Template_Group"/>
> + */
> +public class VirtualGuestBlockDeviceTemplateGroup {
> +
> + public static Builder<?> builder() {
> + return new ConcreteBuilder();
> + }
> +
> + public Builder<?> toBuilder() {
> + return new ConcreteBuilder().fromVirtualGuestBlockDeviceTemplateGroup(this);
> + }
> +
> + public abstract static class Builder<T extends Builder<T>> {
There is no need for the abstract builder pattern if it is not intended to be extended. If subclassing the builder is not a need in the domain classes, I'd change the builders to not be abstract and remove the need for the `ConcreteBuilder` subclasses.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452182
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #896](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/896/) UNSTABLE
Looks like there's a problem with this pull request
[(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/pull/296#issuecomment-37190362
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> /**
> - * pattern where last group matches core speed
> + * number of milliseconds to wait for an order to arrive on the api.
All javadocs should start in uppercase.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11139500
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + for (int i = 0; i < blockDeviceCapacities.size(); i++) {
> + if (i > 0) { devicePosition = i + 1; }
> + blockDevices.add(VirtualGuestBlockDevice.builder()
> + .device(devicePosition + "")
> + .diskImage(VirtualDiskImage.builder()
> + .capacity(blockDeviceCapacities.get(i))
> + .typeId(Type.valueOf(diskType).ordinal())
> + .build())
> + .build());
> + }
> + return blockDevices;
> + }
> +
> + private Optional<OperatingSystem> tryGetOperatingSystemFrom(final String imageId) {
> + Set<OperatingSystem> operatingSystemsAvailable = createObjectOptionsSupplier.get().getVirtualGuestOperatingSystems();
> + return tryFind(from(operatingSystemsAvailable)
I'm in favor of static imports, but when they improve readability. In the case of `from`, when statically imported it's harder to understand what's happening: From `operatingSystemsAvailable ` to what? In this cases I tend to avoid using static imports in favor of a more readable code.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11137710
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -49,6 +53,7 @@
> public class SoftLayerTemplateOptions extends TemplateOptions implements Cloneable {
>
> protected String domainName = "jclouds.org";
> + protected Optional<List<Integer>> blockDevices = Optional.absent();
Add this variable to the `copyTo` method too, so the object is properly cloned.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11137207
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #715](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/715/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-38793368
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
> @@ -135,100 +148,129 @@ public SoftLayerComputeServiceAdapter(SoftLayerApi client,
>
> checkState(orderInSystem, "order for guest %s doesn't have login details within %sms", result,
> Long.toString(guestLoginDelay));
it is actually a good idea. Not sure if any other provider in Jclouds is doing something like that but I guess it is easy to implement
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10299523
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
>
> + private VirtualGuest createVirtualGuest() {
> + return VirtualGuest.builder()
> + .domain("example.com")
> + .hostname("host1")
> + .id(1301396)
> + .maxMemory(1024)
> + .startCpus(1)
> + .operatingSystem(OperatingSystem.builder().id("UBUNTU_LATEST").build())
> + .datacenter(Datacenter.builder().name("test").build())
> + .build();
> }
> }
There are only the expect tests for the list/get/create methods. All other methods in the VirtualGuestApi class should have the corresponding expect tests too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10453210
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + .transform(new Function<VirtualGuestBlockDevice, BlockDevice>() {
> + @Override
> + public BlockDevice apply(VirtualGuestBlockDevice input) {
> + return new BlockDevice(input.getDevice(), input.getVirtualDiskImage().getCapacity());
> + }
> + })
> + .toSortedSet(new Comparator<BlockDevice>() {
> + @Override
> + public int compare(BlockDevice b1, BlockDevice b2) {
> + return Integer.valueOf(b1.getDevice()).compareTo(Integer.valueOf(b2.getDevice()));
> + }
> + });
> + return set;
> + }
> +
> + private HashSet<NetworkComponent> getNetworkComponents(VirtualGuest virtualGuest) {
Even if it is a private method, return the `Set` interface instead of a concrete implementation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11136489
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public Builder fromContainerVirtualGuestConfigurationOption(ContainerVirtualGuestConfigurationOption in) {
> + return this
> + .productItemPrice(in.getProductItemPrice())
> + .template(in.getTemplate());
> + }
> + }
> +
> + private final ProductItemPrice productItemPrice;
> + private final VirtualGuest template;
> +
> + @ConstructorProperties({"itemPrice", "template"})
> + public ContainerVirtualGuestConfigurationOption(ProductItemPrice productItemPrice, VirtualGuest template) {
> + this.productItemPrice = productItemPrice;
> + this.template = template;
Add null checks if parameters are mandatory.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r11138533
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1203](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1203/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-39557706
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -128,4 +135,11 @@ protected void tearDown() {
> protected Iterable<Module> setupModules() {
> return ImmutableSet.<Module> of(getLoggingModule(), new SshjSshClientModule());
> }
> +
> + @Override
> + protected Properties setupProperties() {
> + Properties properties = super.setupProperties();
> + properties.setProperty("jclouds.ssh.max-retries", "10");
> + return properties;
> + }
This class should extend the `BaseComputeServiceLiveTest`. That base class already has all the tests that must pass to properly implement the ComputeService contract. You can take a look at the [digitalocean one](https://github.com/jclouds/jclouds-labs/blob/master/digitalocean/src/test/java/org/jclouds/digitalocean/compute/DigitalOceanComputeServiceLiveTest.java), as it has been the last compute provider added.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10452884
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @ConstructorProperties({
> + "blockDevices", "datacenters", "memory", "networkComponents", "operatingSystems", "processors"
> + })
> + public ContainerVirtualGuestConfiguration(Set<ContainerVirtualGuestConfigurationOption> blockDevices,
> + Set<ContainerVirtualGuestConfigurationOption> datacenters,
> + Set<ContainerVirtualGuestConfigurationOption> memory,
> + Set<ContainerVirtualGuestConfigurationOption> networkComponents,
> + Set<ContainerVirtualGuestConfigurationOption> operatingSystems,
> + Set<ContainerVirtualGuestConfigurationOption> processors) {
> + this.blockDevices = blockDevices;
> + this.datacenters = datacenters;
> + this.memory = memory;
> + this.networkComponents = networkComponents;
> + this.operatingSystems = operatingSystems;
> + this.processors = processors;
Add null checks to all mandatory fields
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296/files#r10276821
Re: [jclouds] Refactor SoftLayer CCI support (#296)
Posted by Andrea Turli <no...@github.com>.
@nacx I've addressed all of your comments but the 'rename API methods' as we are still waiting for opinions from @demobox
The LiveTests are quite ok but still not perfect
Results :
Tests run: 150, Failures: 18, Errors: 0, Skipped: 13
The failures come from `SoftLayerTemplateBuilderLiveTest` but it still unclear to me the needed fix and `SoftLayerComputeServiceLiveTest` which is ok from my IDE :(
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/296#issuecomment-37480308