You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrea Turli <no...@github.com> on 2018/07/05 10:47:49 UTC
[jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/440
-- Commit Summary --
* [JCLOUDS-1430] - add region and zone API
-- File Changes --
M aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/ECSComputeServiceApi.java (4)
M aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/ECSComputeServiceProviderMetadata.java (2)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/DedicatedHostGeneration.java (25)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/Region.java (36)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/ResourceInfo.java (66)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/Zone.java (71)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/features/RegionAndZoneApi.java (58)
A aliyun-ecs/src/test/java/org/jclouds/aliyun/ecs/compute/features/RegionAndZoneApiLiveTest.java (62)
A aliyun-ecs/src/test/java/org/jclouds/aliyun/ecs/compute/features/RegionAndZoneApiMockTest.java (65)
A aliyun-ecs/src/test/resources/regions.json (79)
A aliyun-ecs/src/test/resources/zones.json (477)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/440.patch
https://github.com/jclouds/jclouds-labs/pull/440.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/440
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Ignasi Barrera <no...@github.com>.
nacx requested changes on this pull request.
> RecordedRequest request = server.takeRequest();
assertEquals(request.getMethod(), method);
Map<String, String> queryParameters = Splitter.on('&').trimResults().withKeyValueSeparator("=").split(request.getPath());
assertEquals(queryParameters.get("Action"), action);
+ assertEquals(queryParameters.get("RegionId"), location);
+ assertEquals(request.getHeader("Accept"), "application/json");
+ return request;
+ }
Instead of doing this in a method that should be generic, add a way to pass the query parameters. Otherwise you have a method in the base class that only works for one very concrete query parameter, which is not good.
--
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/440#pullrequestreview-134956737
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> RecordedRequest request = server.takeRequest();
assertEquals(request.getMethod(), method);
Map<String, String> queryParameters = Splitter.on('&').trimResults().withKeyValueSeparator("=").split(request.getPath());
assertEquals(queryParameters.get("Action"), action);
+ assertEquals(queryParameters.get("RegionId"), location);
+ assertEquals(request.getHeader("Accept"), "application/json");
+ return request;
+ }
I think you are right, the 3 methods were a smell for me too. I'll come up with something better
--
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/440#discussion_r200608576
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Ignasi Barrera <no...@github.com>.
Yes, LGTM, although I'd keep the "action" parameter separate and use the map just for the additional ones. The assertions would look more consistent with the methods that don't have extra params.
--
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/440#issuecomment-403001349
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + *
+ * Unless required by applicable law or agreed to in writing, software
+ * 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.aliyun.ecs.domain;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class DedicatedHostGeneration {
+
+ DedicatedHostGeneration() {}
+}
yeah sorry
--
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/440#discussion_r200586080
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + assertEquals(server.getRequestCount(), 1);
+ }
+
+ public void testListZones() throws InterruptedException {
+ server.enqueue(jsonResponse("/zones.json"));
+ List<Zone> zones = api.regionAndZoneApi().describeZones(Regions.EU_CENTRAL_1.getName());
+ assertEquals(size(zones), 2);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeZones");
+ }
+
+ public void testListZonesReturns404() {
+ server.enqueue(response404());
+ List<Zone> zones = api.regionAndZoneApi().describeZones(Regions.EU_CENTRAL_1.getName());
+ assertTrue(isEmpty(zones));
+ assertEquals(server.getRequestCount(), 1);
cool
--
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/440#discussion_r200586108
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
merged at [mater](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/f38f8045)
--
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/440#issuecomment-403009890
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
any better @nacx ? thanks
--
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/440#issuecomment-402994493
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 1 commit.
753df7f more improvements
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/440/files/c3e6c8ce7bcac795620cfebaf93898eef52692dd..753df7f3e6853d008aa290ce6f185c193f728d5f
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + Map<String, List<String>> networkTypes) {
+ return new AutoValue_ResourceInfo(ioOptimized, systemDiskCategories == null ?
+ ImmutableMap.<String, List<String>>of() :
+ ImmutableMap.copyOf(systemDiskCategories),
+ instanceTypes == null ? ImmutableMap.<String, List<String>>of() : ImmutableMap.copyOf(instanceTypes),
+ instanceTypeFamilies == null ?
+ ImmutableMap.<String, List<String>>of() :
+ ImmutableMap.copyOf(instanceTypeFamilies), dataDiskCategories == null ?
+ ImmutableMap.<String, List<String>>of() :
+ ImmutableMap.copyOf(dataDiskCategories), instanceGenerations == null ?
+ ImmutableMap.<String, List<String>>of() :
+ ImmutableMap.copyOf(instanceGenerations),
+ networkTypes == null ? ImmutableMap.<String, List<String>>of() : ImmutableMap.copyOf(networkTypes));
+ }
+
+ public abstract Boolean ioOptimized();
Use primitives if not nullable
> +package org.jclouds.aliyun.ecs.compute.features;
+
+import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
+import org.jclouds.aliyun.ecs.compute.internal.BaseECSComputeServiceApiLiveTest;
+import org.jclouds.aliyun.ecs.domain.Region;
+import org.jclouds.aliyun.ecs.domain.Zone;
+import org.jclouds.aliyun.ecs.features.RegionAndZoneApi;
+import org.testng.annotations.Test;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.testng.Assert.assertTrue;
+import static org.testng.util.Strings.isNullOrEmpty;
+
+@Test(groups = "live", testName = "InstanceApiLiveTest")
Fix test name
> +@Test(groups = "unit", testName = "RegionAndZoneApiMockTest", singleThreaded = true)
+public class RegionAndZoneApiMockTest extends BaseECSComputeServiceApiMockTest {
+
+ public void testListRegions() throws InterruptedException {
+ server.enqueue(jsonResponse("/regions.json"));
+ List<Region> regions = api.regionAndZoneApi().describeRegions();
+ assertEquals(size(regions), 18);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeRegions");
+ }
+
+ public void testListRegionsReturns404() {
+ server.enqueue(response404());
+ List<Region> regions = api.regionAndZoneApi().describeRegions();
+ assertTrue(isEmpty(regions));
+ assertEquals(server.getRequestCount(), 1);
Assert the request being sent
> + assertSent(server, "GET", "DescribeRegions");
+ }
+
+ public void testListRegionsReturns404() {
+ server.enqueue(response404());
+ List<Region> regions = api.regionAndZoneApi().describeRegions();
+ assertTrue(isEmpty(regions));
+ assertEquals(server.getRequestCount(), 1);
+ }
+
+ public void testListZones() throws InterruptedException {
+ server.enqueue(jsonResponse("/zones.json"));
+ List<Zone> zones = api.regionAndZoneApi().describeZones(Regions.EU_CENTRAL_1.getName());
+ assertEquals(size(zones), 2);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeZones");
Looks incomplete... You are specifying a parameter and we should be verifying the exact request.
> + assertEquals(server.getRequestCount(), 1);
+ }
+
+ public void testListZones() throws InterruptedException {
+ server.enqueue(jsonResponse("/zones.json"));
+ List<Zone> zones = api.regionAndZoneApi().describeZones(Regions.EU_CENTRAL_1.getName());
+ assertEquals(size(zones), 2);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeZones");
+ }
+
+ public void testListZonesReturns404() {
+ server.enqueue(response404());
+ List<Zone> zones = api.regionAndZoneApi().describeZones(Regions.EU_CENTRAL_1.getName());
+ assertTrue(isEmpty(zones));
+ assertEquals(server.getRequestCount(), 1);
Verify the request being sent
> + *
+ * Unless required by applicable law or agreed to in writing, software
+ * 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.aliyun.ecs.domain;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class DedicatedHostGeneration {
+
+ DedicatedHostGeneration() {}
+}
Unused class?
--
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/440#pullrequestreview-134599629
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + assertSent(server, "GET", "DescribeRegions");
+ }
+
+ public void testListRegionsReturns404() {
+ server.enqueue(response404());
+ List<Region> regions = api.regionAndZoneApi().describeRegions();
+ assertTrue(isEmpty(regions));
+ assertEquals(server.getRequestCount(), 1);
+ }
+
+ public void testListZones() throws InterruptedException {
+ server.enqueue(jsonResponse("/zones.json"));
+ List<Zone> zones = api.regionAndZoneApi().describeZones(Regions.EU_CENTRAL_1.getName());
+ assertEquals(size(zones), 2);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeZones");
You are invoking the method with one parameter, that will end up somewhere in the request. This assertion will succeed regardless of the parameter value, or even if there is no parameter at all preset (bad!) which would not verify the proper behavior of the method and could lead to false positives.
--
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/440#discussion_r200588490
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + assertSent(server, "GET", "DescribeRegions");
+ }
+
+ public void testListRegionsReturns404() {
+ server.enqueue(response404());
+ List<Region> regions = api.regionAndZoneApi().describeRegions();
+ assertTrue(isEmpty(regions));
+ assertEquals(server.getRequestCount(), 1);
+ }
+
+ public void testListZones() throws InterruptedException {
+ server.enqueue(jsonResponse("/zones.json"));
+ List<Zone> zones = api.regionAndZoneApi().describeZones(Regions.EU_CENTRAL_1.getName());
+ assertEquals(size(zones), 2);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeZones");
you mean `Regions.EU_CENTRAL_1.getName()` ? Ok I will add the check for location then in the `assertSent` to be more generic
--
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/440#discussion_r200589484
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.
--
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/440#pullrequestreview-134980120
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + Map<String, List<String>> networkTypes) {
+ return new AutoValue_ResourceInfo(ioOptimized, systemDiskCategories == null ?
+ ImmutableMap.<String, List<String>>of() :
+ ImmutableMap.copyOf(systemDiskCategories),
+ instanceTypes == null ? ImmutableMap.<String, List<String>>of() : ImmutableMap.copyOf(instanceTypes),
+ instanceTypeFamilies == null ?
+ ImmutableMap.<String, List<String>>of() :
+ ImmutableMap.copyOf(instanceTypeFamilies), dataDiskCategories == null ?
+ ImmutableMap.<String, List<String>>of() :
+ ImmutableMap.copyOf(dataDiskCategories), instanceGenerations == null ?
+ ImmutableMap.<String, List<String>>of() :
+ ImmutableMap.copyOf(instanceGenerations),
+ networkTypes == null ? ImmutableMap.<String, List<String>>of() : ImmutableMap.copyOf(networkTypes));
+ }
+
+ public abstract Boolean ioOptimized();
good point, thanks
--
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/440#discussion_r200585746
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + assertSent(server, "GET", "DescribeRegions");
+ }
+
+ public void testListRegionsReturns404() {
+ server.enqueue(response404());
+ List<Region> regions = api.regionAndZoneApi().describeRegions();
+ assertTrue(isEmpty(regions));
+ assertEquals(server.getRequestCount(), 1);
+ }
+
+ public void testListZones() throws InterruptedException {
+ server.enqueue(jsonResponse("/zones.json"));
+ List<Zone> zones = api.regionAndZoneApi().describeZones(Regions.EU_CENTRAL_1.getName());
+ assertEquals(size(zones), 2);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeZones");
not sure I follow, in this case I'm not using any options so I think it's better to check the action sent. wdyt?
--
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/440#discussion_r200586043
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +package org.jclouds.aliyun.ecs.compute.features;
+
+import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
+import org.jclouds.aliyun.ecs.compute.internal.BaseECSComputeServiceApiLiveTest;
+import org.jclouds.aliyun.ecs.domain.Region;
+import org.jclouds.aliyun.ecs.domain.Zone;
+import org.jclouds.aliyun.ecs.features.RegionAndZoneApi;
+import org.testng.annotations.Test;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.testng.Assert.assertTrue;
+import static org.testng.util.Strings.isNullOrEmpty;
+
+@Test(groups = "live", testName = "InstanceApiLiveTest")
ops, thanks
--
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/440#discussion_r200585782
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 1 commit.
5dc166e address comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/440/files/f2e15c37bbc7ff71f7ebdc992d9387b7ba41b816..5dc166e56659bae37f57601c55d4073c13e25fc9
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
thanks @nacx
--
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/440#issuecomment-403009918
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 1 commit.
c3e6c8c better assertSent
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/440/files/5dc166e56659bae37f57601c55d4073c13e25fc9..c3e6c8ce7bcac795620cfebaf93898eef52692dd
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
squashed and merging
--
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/440#issuecomment-403008358
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] - add region and zone API
(#440)
Posted by Andrea Turli <no...@github.com>.
Closed #440.
--
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/440#event-1720091784