Re: [jclouds] fix support for private images in SoftLayer (#568)
@demobox, I've cherry-picked the commit onto master, shall I close the PR manually? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59170282
Re: [jclouds] fix support for private images in SoftLayer (#568)
Closed #568. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#event-179079167
Re: [jclouds] fix support for private images in SoftLayer (#568)
Thanks @adriancole @demobox, I've addressed your comments and I've got a LGTM so good to merge --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59002034
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1289](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1289/) FAILURE 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/568#issuecomment-59004492
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds » jclouds #1793](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1793/) FAILURE 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/568#issuecomment-59007053
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests-java-6 #201](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/201/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59042279
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1290](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1290/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59047923
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds » jclouds #1794](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1794/) 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/568#issuecomment-59052622
Re: [jclouds] fix support for private images in SoftLayer (#568)
:+1: LGTM --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-59127997
Re: [jclouds] fix support for private images in SoftLayer (#568)
Thanks @demobox Main reason for this cleanup is that the private image support is currently `broken` so it would be nice to have that merged soon. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58907317
Re: [jclouds] fix support for private images in SoftLayer (#568)
+ });
+ if (!optionalBootableVolume.isPresent()) {
+ return Type.LOCAL.ordinal();
+ }
+ return optionalBootableVolume.get().getType().ordinal();
+ }
+
+ private OptionalOperatingSystem tryExtractOperatingSystemFrom(final
String imageId) {
+ SetOperatingSystem operatingSystemsAvailable =
createObjectOptionsSupplier.get().getVirtualGuestOperatingSystems();
+ return tryFind(FluentIterable.from(operatingSystemsAvailable)
+ .filter(new PredicateOperatingSystem() {
+ @Override
+ public boolean apply(OperatingSystem input) {
+return input.getId().contains(imageId);
+ }
+ }), Predicates.notNull());
agreed. the not-null filter is confusing, as you would have gotten a NPE here,
at `input.getId()` right?
```java
+return input.getId().contains(imageId);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18778892
Re: [jclouds] fix support for private images in SoftLayer (#568)
+ private final String osReferenceCode;
+
+ public HasSameOsReferenceCode(String osReferenceCode) {
+ this.osReferenceCode = osReferenceCode;
+ }
+
+ @Override
+ public boolean apply(SoftwareDescription input) {
+ return input.getReferenceCode().equals(osReferenceCode);
+ }
+ }
+
+ private class IsOperatingSystem implements PredicateSoftwareDescription
{
+ @Override
+ public boolean apply(SoftwareDescription softwareDescription) {
+ return softwareDescription.getOperatingSystem() == 1;
agreed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18778919
Re: [jclouds] fix support for private images in SoftLayer (#568)
Made some notes of things I care about, which addressed == LGTM --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58917002
Re: [jclouds] fix support for private images in SoftLayer (#568)
PS @andreaturli since you are a committer, once you have a LGTM, cherry-pick this into master and 1.8.x on your own. You have the power! --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58916605
Re: [jclouds] fix support for private images in SoftLayer (#568)
@@ -445,8 +355,66 @@ public void suspendNode(String id) {
api.getVirtualGuestApi().pauseVirtualGuest(Long.parseLong(id));
}
- private boolean isOperatingSystem(SoftwareDescription input) {
- return input.getOperatingSystem() == 1;
+ /**
+* This method will deliberately skip device position 1 as it is reserved
to SWAP
+* @param blockDeviceCapacities list of blockDevices to be attached
+* @param diskType disks can be LOCAL or SAN
+* @return
+*/
+ private ListVirtualGuestBlockDevice getBlockDevices(ListInteger
blockDeviceCapacities, String diskType) {
+ ListVirtualGuestBlockDevice blockDevices = Lists.newArrayList();
[minor] Does the result need to me a mutable list? If not, use
`ImmutableList.builder()`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18748117
Re: [jclouds] fix support for private images in SoftLayer (#568)
@@ -445,8 +355,66 @@ public void suspendNode(String id) {
api.getVirtualGuestApi().pauseVirtualGuest(Long.parseLong(id));
}
- private boolean isOperatingSystem(SoftwareDescription input) {
- return input.getOperatingSystem() == 1;
+ /**
+* This method will deliberately skip device position 1 as it is reserved
to SWAP
+* @param blockDeviceCapacities list of blockDevices to be attached
+* @param diskType disks can be LOCAL or SAN
+* @return
+*/
+ private ListVirtualGuestBlockDevice getBlockDevices(ListInteger
blockDeviceCapacities, String diskType) {
[minor] Can be `static`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18748113
Re: [jclouds] fix support for private images in SoftLayer (#568)
+ });
+ if (!optionalBootableVolume.isPresent()) {
+ return Type.LOCAL.ordinal();
+ }
+ return optionalBootableVolume.get().getType().ordinal();
+ }
+
+ private OptionalOperatingSystem tryExtractOperatingSystemFrom(final
String imageId) {
+ SetOperatingSystem operatingSystemsAvailable =
createObjectOptionsSupplier.get().getVirtualGuestOperatingSystems();
+ return tryFind(FluentIterable.from(operatingSystemsAvailable)
+ .filter(new PredicateOperatingSystem() {
+ @Override
+ public boolean apply(OperatingSystem input) {
+return input.getId().contains(imageId);
+ }
+ }), Predicates.notNull());
[minor] What is the `notNull()` for? Does `operatingSystemsAvailable` possible
contain `null` elements? And just to check I'm understanding the logic
correctly here: the image ID can be a substring of the the ID of an
OperatingSystem, and that is what we are looking?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18748139
Re: [jclouds] fix support for private images in SoftLayer (#568)
+ private static class IsBootableDevice implements
PredicateVirtualGuestBlockDeviceTemplate {
+ @Override
+ public boolean apply(VirtualGuestBlockDeviceTemplate
blockDeviceTemplate) {
+ return blockDeviceTemplate.getDevice().equals(BOOTABLE_DEVICE);
+ }
+ }
+
+ private static class
BlockDeviceTemplateGroupToBlockDeviceTemplateIterable implements
FunctionVirtualGuestBlockDeviceTemplateGroup,
+ IterableVirtualGuestBlockDeviceTemplate {
+ @Override
+ public IterableVirtualGuestBlockDeviceTemplate
apply(VirtualGuestBlockDeviceTemplateGroup input) {
+ return input.getBlockDevices();
+ }
+ }
+
+ private static class VirtualGuestBlockDeviceTemplateIterableFunction
implements FunctionVirtualGuestBlockDeviceTemplate,
IterableVirtualDiskImageSoftware {
[minor] As with the other functions, name this so it is called `XToY`, e.g.
`BlockDeviceTemplateToDiskImageSoftware` or so?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18748146
Re: [jclouds] fix support for private images in SoftLayer (#568)
+ .filter(new PredicateOperatingSystem() {
+ @Override
+ public boolean apply(OperatingSystem input) {
+return input.getId().contains(imageId);
+ }
+ }), Predicates.notNull());
+ }
+
+ private OptionalOperatingSystem
tryExtractOperatingSystemFrom(VirtualGuestBlockDeviceTemplateGroup image) {
+ return FluentIterable.from(image.getChildren())
+ .transformAndConcat(new
BlockDeviceTemplateGroupToBlockDeviceTemplateIterable())
+ .filter(new IsBootableDevice())
+ .transformAndConcat(new
VirtualGuestBlockDeviceTemplateIterableFunction())
+ .transform(new DiskImageSoftwareToSoftwareDescription())
+ .filter(new IsOperatingSystem())
+ .transform(new
SoftwareDescriptionToOperatingSystem(image.getGlobalIdentifier()))
Do all the functions being passed here need to be instantiated each time? Or
could we use a singleton for them?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18748155
Re: [jclouds] fix support for private images in SoftLayer (#568)
+ private final String osReferenceCode;
+
+ public HasSameOsReferenceCode(String osReferenceCode) {
+ this.osReferenceCode = osReferenceCode;
+ }
+
+ @Override
+ public boolean apply(SoftwareDescription input) {
+ return input.getReferenceCode().equals(osReferenceCode);
+ }
+ }
+
+ private class IsOperatingSystem implements PredicateSoftwareDescription
{
+ @Override
+ public boolean apply(SoftwareDescription softwareDescription) {
+ return softwareDescription.getOperatingSystem() == 1;
Could you add a comment to describe what the value of `1` means here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18748156
Re: [jclouds] fix support for private images in SoftLayer (#568)
@@ -67,15 +67,18 @@ public void testLaunchClusterWithMinDisk() throws
RunNodesException {
TemplateBuilder templateBuilder =
context.getComputeService().templateBuilder();
templateBuilder.imageId(CENTOS_6_64);
+ //templateBuilder.imageVersionMatches(6.5);
Here and below: are these commented-out on purpose? If so, remove?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18748161
Re: [jclouds] fix support for private images in SoftLayer (#568)
Thanks for this, @andreaturli! Most of my comments we can take care of during merging, I guess the main one is whether we can create singletons for the various new transformation functions this PR adds. For background: could you explain what the motivation for the PR is? Is private image support currently _broken_, or is this mainly a cleanup, or..? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58804644
[jclouds] fix support for private images in SoftLayer (#568)
You can merge this Pull Request by running: git pull https://github.com/andreaturli/jclouds fix/private-images Or you can view, comment on it, or merge it online at: https://github.com/jclouds/jclouds/pull/568 -- Commit Summary -- * fix support for private images in SoftLayer -- File Changes -- M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/strategy/SoftLayerComputeServiceAdapter.java (20) M providers/softlayer/src/test/java/org/jclouds/softlayer/compute/SoftLayerComputeServiceContextLiveTest.java (2) -- Patch Links -- https://github.com/jclouds/jclouds/pull/568.patch https://github.com/jclouds/jclouds/pull/568.diff --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests-java-6 #188](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/188/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58629131
Re: [jclouds] fix support for private images in SoftLayer (#568)
@@ -339,7 +339,9 @@ public boolean apply(SoftwareDescription input) {
private OperatingSystem getOperatingSystem(Map.EntryString,
SoftwareDescription entry) {
SoftwareDescription softwareDescription = entry.getValue();
- if (isOperatingSystem(softwareDescription)) {
+ if (!isOperatingSystem(softwareDescription)) {
+ return null;
I think this method should be changed:
* Why does it receive a `Map.Entry` parameter? Change it to receive a String
and a SoftwareDescription object, to make its signature more clean and
meaningful.
* Also, returning `null` is evil. Avoid that. As this is a private method only
used to populate values in a Set, just make it return void and refactor to
something like `addOperatingSystemToSet`, and pass the Set as a parameter too.
This way the method logic is self-contained and you're not propagating details
such as the null value (which can introduce NPE issues) to the outside.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18695140
Re: [jclouds] fix support for private images in SoftLayer (#568)
private MapString, SoftwareDescription
extractSoftwareDescriptions(SetVirtualGuestBlockDeviceTemplateGroup images)
{
MapString, SoftwareDescription softwareDescriptions =
Maps.newHashMap();
for (VirtualGuestBlockDeviceTemplateGroup image : images) {
final String globalIdentifier = image.getGlobalIdentifier();
for (VirtualGuestBlockDeviceTemplateGroup child :
image.getChildren()) {
for (VirtualGuestBlockDeviceTemplate blockDeviceTemplate :
child.getBlockDevices()) {
- for (VirtualDiskImageSoftware softwareReference :
blockDeviceTemplate.getDiskImage().getSoftwareReferences()) {
- softwareDescriptions.put(globalIdentifier,
softwareReference.getSoftwareDescription());
+ // filter for the device 0 (bootable)
+ if (blockDeviceTemplate.getDevice().equals(0)) {
Wirth refactoring to a reusable `isBootableDevice` predicate, and use it to
filter the `child.getBlockDevices()` collection in the loop, instead of having
this `if` statement?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/568/files#r18695190
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1277](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1277/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58631814
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests-java-6 #189](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/189/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58666264
Re: [jclouds] fix support for private images in SoftLayer (#568)
thank @nacx I took the occasion to refactor listImages a bit more, now looks to me simpler to read and maintain. Thoughts? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58666540
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1278](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1278/) FAILURE 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/568#issuecomment-58670156
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds » jclouds #1774](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1774/) FAILURE 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/568#issuecomment-58673274
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests-java-6 #190](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/190/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58680089
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds-pull-requests #1279](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1279/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/568#issuecomment-58683136
Re: [jclouds] fix support for private images in SoftLayer (#568)
[jclouds » jclouds #1775](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1775/) 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/568#issuecomment-58687619
