Add AttachedDisk class, subclasses and tests

[mziccard]

This is an alternative PR to #917 for the sake of discussion.
Instead of having a single AttachedDisk class and 3 configuration objects this class moves the hierarchy to AttachedDisk, creating 3 subclasses PersistentAttachedDisk, CreateAttachedDisk and ScratchAttachedDisk.

PROs of this approach with respect to #917 is that we can write attachDisk method as:

Operation attachDisk(InstanceId instance, PersistentDiskConfiguration configuration, OperationOption... options);

Operation attachDisk(InstanceId instance, PersistentAttachedDisk configuration,
    OperationOption... options);

And the InstanceInfo.of methods will become:

InstanceInfo of(InstanceId instance, MachineTypeId machineType,
    CreateAttachedDisk attachedDisk, NetworkInterface networkinterface);
InstanceInfo of(InstanceId instance, MachineTypeId machineType,
    PersistentAttachedDisk attachedDisk, NetworkInterface networkinterface);

Thus capturing the fact that an instance cannot be created with only one scratch disk.

CONS of this approach is that once we get an instance getting all information on it's attached disks requires casting:

Instance instance = compute.get(...);
AttachedDisk attachedDisk = instance.attachedDisks().get(0);
if (attachedDisk.type() == AttachedDisk.Type.PERSISTENT) {
  PersistentAttachedDisk persistentAttachedDisk = (PersistentAttachedDisk) attachedDisk;
  // use fields of a persistent attached disk
}

Any thoughts would be appreciated :) /cc @mderka @aozarov @ajkannan

[coveralls]

Coverage increased (+0.2%) to 83.414% when pulling 1af6ff5 on mziccard:compute-attached-disk2 into 3fe785b on GoogleCloudPlatform:compute-alpha.

[aozarov]

PROs of this approach with respect to #917 is that we can write attachDisk method as:
Operation attachDisk(InstanceId instance, PersistentDiskConfiguration configuration,
OperationOption... options);

Did you mean Operation attachDisk(InstanceId instance, AttachedDisk attachedDisk, OpeationOptions... options);? or am I missing something?

[mziccard]

Yes sorry I meant:

Operation attachDisk(InstanceId instance, PersistentAttachedDisk attachedDisk, OperationOptions... options);

Fixing it now.

[aozarov]

CONS of this approach is that once we get an instance getting all information on it's attached disks requires casting:

Isn't that the same for getting the configuration from an AttachedDisk in #917 ?
Also, because one could not infer the the instance type based on the AttachedDiskConfiguration.Type enum I am not sure how useful (or dangerous) the auto-cast AttachedDisk.configuration method is.

Also, given your example above instance.attachedDisks().get(0); how one would know that it is a CreateAttachedDisk instance (or is that only used when creating an instance or attaching a disk) and never for when getting an instance (I guess answer also applies to the comment above)?

[mziccard]

Isn't that the same for getting the configuration from an AttachedDisk in #917 ?

Yes you still need casting, however in this case it's explicit.

or is that only used when creating an instance or attaching a disk

Yes CreateAttachedDisk is never returned. It's used only for creation and then the same disk is returned from the service as a PersistentAttachedDisk.

[aozarov]

Added a comment in #917 regarding the set of types that can be passed to attachDisk.

Also, I do wonder if CreateAttachedDisk should be a subclass of an AttachedDisk considering that
it is not an actual type and therefore never going to be return as value. I do understand why you do that but also find it confusing.

For these reasons and for the one you mentioned I think I prefer #917. I think the cost of being
able to pass more disk-configurations than what (currently) allowed is a small price to pay.
Hopefully the server error in that case is clear?

[mziccard]

I think the cost of being
able to pass more disk-configurations than what (currently) allowed is a small price to pay.
Hopefully the server error in that case is clear?

I think it's quite clear:

{
 "error": {
  "errors": [
   {
    "domain": "global",
    "reason": "invalid",
    "message": "Invalid value for field 'resource.disks[0]': ''. Boot disk must be the first disk attached to the instance."
   }
  ],
  "code": 400,
  "message": "Invalid value for field 'resource.disks[0]': ''. Boot disk must be the first disk attached to the instance."
 }
}

[aozarov]

which makes my preference to even stronger #917. Also, per comment in the other PR, would be worth to know if that is a doc error (suggesting more possibilities) and/or plans to support it at some point.

[mziccard]

Closing this as we decided to go with #917