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
[jclouds/jclouds-labs] JCLOUDS-1407 - Dimensiondata Server API 2.6 support (#434)
Server API 2.6 support implementation You can view, comment on, or merge this pull request online at: https://github.com/jclouds/jclouds-labs/pull/434 -- Commit Summary -- * Server API 2.6 support -- File Changes -- A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/AbstractBaseController.java (31) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/AbstractDrive.java (30) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/Floppy.java (67) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/IdeController.java (72) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/IdeDevice.java (60) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/IdeDisk.java (54) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/SataController.java (72) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/SataDevice.java (60) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/SataDisk.java (54) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/ScsiController.java (72) A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/ScsiDisk.java (55) M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/Server.java (61) M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/options/CreateServerOptions.java (3) M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/features/ServerApi.java (2) M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/features/ServerApiLiveTest.java (26) M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/features/ServerApiMockTest.java (26) M dimensiondata/src/test/resources/server.json (157) -- Patch Links -- https://github.com/jclouds/jclouds-labs/pull/434.patch https://github.com/jclouds/jclouds-labs/pull/434.diff -- 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
