jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] JCLOUDS-1406 - Add default location configuration to dimension data provider (#433)
Date Thu, 12 Apr 2018 12:48:16 GMT
nacx commented on this pull request.

> @@ -40,6 +43,17 @@
 public class DimensionDataCloudControlHttpApiModule extends HttpApiModule<DimensionDataCloudControlApi>
+   @Override
+   protected void installLocations() {
+      super.installLocations();
+      //      bind(RegionIdToZoneIdsSupplier.class).to(ZonesForRegion.class).in(Scopes.SINGLETON);
+      bind(RegionIdToURISupplier.class).to(RegionsToApiEndpoints.class).in(Scopes.SINGLETON);
+      //      bind(ZoneIdsSupplier.class).to(ZoneIdsFromRegionIdToZoneIdsValues.class).in(Scopes.SINGLETON);
+      //      bind(RegionIdsSupplier.class).to(RegionIdsFromRegionIdToURIKeySet.class).in(Scopes.SINGLETON);
+      //      bind(ZoneIdToURISupplier.class).to(ZoneIdToURIFromJoinOnRegionIdToURI.class).in(Scopes.SINGLETON);
+      //      bind(ImplicitLocationSupplier.class).to(OnlyLocationOrFirstZone.class).in(Scopes.SINGLETON);

I'd say you need this four supplier bindings, as the endpoints are "per-region", not "per-zone".

>        if (datacenterIds != null && !datacenterIds.isEmpty()) {
-         return request.toBuilder().addQueryParam("datacenterId", datacenterIds).build();
+         return request.toBuilder().addQueryParam(queryParam, datacenterIds).build();
       } else {
          return request;

Having this as a filter might be problematic in the future, as the filter works with the "default
region". When users start using the ComputeService and configure the TempalteOptions to deploy
to a concrete region, there would be no way to call the InfraApi (or any API using this filter)
and make it filter by the configured region.
It would be better to remove this filter and add the datacenterId as method parameters in
the API.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
View raw message