Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
Closed #434. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/434#event-1886763269
Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
New PR (https://github.com/jclouds/jclouds-labs/pull/447) created instead of this one, as it's quite outdated -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/434#issuecomment-426999486
Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
btrishkin commented on this pull request. > - @Nullable - public abstract List deviceOrDisks(); Yes, AbstractDrive will help a lot in ServerToHardware function implementation for a new ServerApi version. Gonna keep it then. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/434#discussion_r182699242
Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
nacx commented on this pull request. > - @Nullable - public abstract List deviceOrDisks(); Oh, I see now that inheritance here was there not only to reuse fields but because the API returns collections with different object types in it. To avoid having a `List`, which makes the API hard to consume, would it make sense to have just one object with all fields? I see the child classes just had one or two different fields. Would that make sense? If not, I'd consider getting the abstract classes back and just be extra-careful in variable order. take into account that the fields in the base class are serialized first, then the ones in the child class (and if at some point the base class is modified, the annotations in all child classes need to be updated accordingly). Apologies for the noise if you consider the second option to be better, but till now it wasn't clear for me the purpose of the abstract classes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/434#pullrequestreview-113545506
Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
@btrishkin pushed 1 commit. 904fcbd Server API 2.6 support -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/434/files/28a17e29e26ee710272e19c42d2bd9e2a6f5f1ee..904fcbdb9c8c305f7dd119169ce0eededcaf3599
Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
nacx commented on this pull request. > @@ -61,7 +61,7 @@ @RequestFilters({ BasicAuthentication.class, OrganisationIdFilter.class }) @Consumes(MediaType.APPLICATION_JSON) -@Path("/{jclouds.api-version}/server") +@Path("/2.6/server") Understood. Let's leave it as-is then for now. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/434#discussion_r182320113
Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
btrishkin commented on this pull request. > @@ -61,7 +61,7 @@ @RequestFilters({ BasicAuthentication.class, OrganisationIdFilter.class }) @Consumes(MediaType.APPLICATION_JSON) -@Path("/{jclouds.api-version}/server") +@Path("/2.6/server") Well, we implement them in chunks. This PR covers only ServerApi feature which will be updated to new version. Everything else remain old 2.4 for now. We can introduce constant with newest API version supported, but outdated features will have to have 2.4 version hardcoded in them until we implement latest version of them. Would that be ok? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/434#discussion_r182137110
Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
nacx commented on this pull request. > @@ -61,7 +61,7 @@ @RequestFilters({ BasicAuthentication.class, OrganisationIdFilter.class }) @Consumes(MediaType.APPLICATION_JSON) -@Path("/{jclouds.api-version}/server") +@Path("/2.6/server") Do all APIs need to us ethe same version? If so, youd it make sense to store the version in a constant and reference it from all APIs? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/434#discussion_r182133211
Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
btrishkin commented on this pull request. > @@ -61,7 +61,7 @@ @RequestFilters({ BasicAuthentication.class, OrganisationIdFilter.class }) @Consumes(MediaType.APPLICATION_JSON) -@Path("/{jclouds.api-version}/server") +@Path("/2.6/server") Our API have different implementations for different versions, so we cannot simply provide 2.4 version while calling 2.6 ServerApi (the same for all other features that we have (Account, Infrastructure, Network, ServerImage, Tag)). So probably this version will be better hardcoded to reflect latest dimensiondata API version supported by jclouds (it is 2.4 now in jclouds while latest released dimensiondata version is 2.7 and we're going to try and update everything to reflect that). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/434#discussion_r182087364
Re: [jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
nacx requested changes on this pull request. Some general comments: * Prefer primitive types when non-nullable. * Try to avoid null collections; initialize to empty. * Avoid inheritance as Google Auto does not really support it. * Declare the fields in the same order they appear in the factory method. Thanks! > + * 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.dimensiondata.cloudcontrol.domain; + +public abstract class AbstractBaseController { + + public abstract String id(); + + public abstract String adapterType(); + + public abstract Integer virtualControllerId(); + + public abstract Integer key(); If these fields are nullable, annotate them accordingly. Otherwise prefer primitive types. > + * 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.dimensiondata.cloudcontrol.domain; + +import org.jclouds.javax.annotation.Nullable; + +public abstract class AbstractDrive { + + public abstract String id(); + + @Nullable + public abstract Integer sizeGb(); + + public abstract String state(); Is there a fixed set of values we could extract into an enum? > + * limitations under the License. + */ +package org.jclouds.dimensiondata.cloudcontrol.domain; + +import com.google.auto.value.AutoValue; +import org.jclouds.javax.annotation.Nullable; +import org.jclouds.json.SerializedNames; + +@AutoValue +public abstract class Floppy { + + public abstract String id(); + + public abstract Integer driveNumber(); + + public abstract Integer virtualFloppyId(); Annotate nullable or prefer primitives. > +package org.jclouds.dimensiondata.cloudcontrol.domain; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import org.jclouds.javax.annotation.Nullable; +import org.jclouds.json.SerializedNames; + +import java.util.List; + +@AutoValue +public abstract class IdeController extends AbstractBaseController { + + public abstract Integer channel(); + + @Nullable + public abstract List deviceOrDisks(); In general, we should prefer non-nullable collections. Can we default to an empty list in the constructor/builder methods when the value comes `null` from the server, or is there a mandatory reason to leave it null? > + * 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.dimensiondata.cloudcontrol.domain; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import org.jclouds.javax.annotation.Nullable; +import org.jclouds.json.SerializedNames; + +import java.util.List; + +@AutoValue +public abstract class IdeController extends AbstractBaseController { Inheritance with Google auto value is not supported. You already have to define the parent class fields in the child class factory methods and builders, wo there is little gain abstracting the common accessors in the base class. There are just a few, so consider removing the base calss and putting all information here. Also, inheritance is tricky: we annotate the "factory methods" with the json field names, not the individual fields. This is good for deserialization but when serializing we need to match each object field to its json name. Ideally we would annotate each field method, but that would mean duplicating the information from the factory method annotation. Instead, assuming the fact that Google Auto does not support inheritance, in jclouds we took the convention that object fields (well, abstract field accessors) would be decalred *in the same order* than the one of the factory method, and jclouds uses that convention when serializing. When there is a class hierarchy, field order is not that clear and more error-prone. > + * 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.dimensiondata.cloudcontrol.domain; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import org.jclouds.javax.annotation.Nullable; +import org.jclouds.json.SerializedNames; + +import java.util.List; + +@AutoValue +public abstract class IdeController extends AbstractBaseController { Just seeing the classes below, you need to configure the order of the parameters in the factory methods matching the order in which