You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Daniel Estévez <no...@github.com> on 2018/07/09 18:33:52 UTC
[jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi and
PublicIPAddressApi (#1225)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1225
-- Commit Summary --
* Adds listAll to PublicIPAddressAPI
* Adds listAll to NetworkInterfaceCardApi
-- File Changes --
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/features/NetworkInterfaceCardApi.java (16)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/features/PublicIPAddressApi.java (16)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/NetworkInterfaceCardApiLiveTest.java (6)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/NetworkInterfaceCardApiMockTest.java (23)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/PublicIPAddressApiLiveTest.java (10)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/PublicIPAddressApiMockTest.java (45)
A providers/azurecompute-arm/src/test/resources/PublicIPAddressListAll.json (102)
A providers/azurecompute-arm/src/test/resources/listallnetworkinterfaces.json (100)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1225.patch
https://github.com/jclouds/jclouds/pull/1225.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/pull/1225
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Ignasi Barrera <no...@github.com>.
Merged to [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/566ac233) and [2.1.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/2bddbd51). Thanks @danielestevez!
--
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/pull/1225#issuecomment-404570618
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
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/pull/1225#pullrequestreview-136029939
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
Thanks @danielestevez!
> @@ -105,6 +105,12 @@ public void listNetworkInterfaceCards() {
assertTrue(nicList.contains(api().get(nicName)));
}
+ @Test(dependsOnMethods = "createNetworkInterfaceCard")
+ public void listAllNetworkInterfaceCards() {
Make the delete test depend on this method too, to avoid deleting the networks first, causing this test to fail.
> + String path = String
+ .format("/subscriptions/%s/providers/Microsoft.Network/networkInterfaces?%s", subscriptionid, apiVersion);
+
+ assertSent(server, "GET", path);
+ assertTrue(nicList.size() == 3);
+ assertTrue(nicList.get(0).properties().ipConfigurations().size() > 0);
+ assertEquals(nicList.get(0).properties().ipConfigurations().get(0).properties().privateIPAllocationMethod(),
+ "Dynamic");
+ assertTrue(nicList.get(1).properties().ipConfigurations().size() > 0);
+ assertEquals(nicList.get(1).properties().ipConfigurations().get(0).properties().privateIPAllocationMethod(),
+ "Static");
+ assertTrue(nicList.get(2).properties().ipConfigurations().size() > 0);
+ assertNotEquals(IdReference.extractResourceGroup(nicList.get(2).id()), resourcegroup);
+ assertEquals(nicList.get(2).properties().ipConfigurations().get(0).properties().privateIPAllocationMethod(),
+ "Static");
+ }
Also add a test that exercises the 404 fallback defined for this new method.
> @@ -119,6 +119,16 @@ public void listPublicIPAddresses() {
assertTrue(ipList.size() > 0);
}
+ @Test(groups = "live", dependsOnMethods = "createPublicIPAddress")
+ public void listAllPublicIPAddresses() {
Same here. Add it as a dependency to the delete test.
> @SelectJson("value")
@GET
@Fallback(EmptyListOnNotFoundOr404.class)
List<NetworkInterfaceCard> list();
+ @Named("networkinterfacecard:list_all")
+ @Path("/providers/Microsoft.Network/networkInterfaces")
+ @SelectJson("value")
+ @GET
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ List<NetworkInterfaceCard> listAll();
The method name looks confusing, as `list` usually means "list all". I think it would make sense to rename it to `listAllInSubscription`, or similar. WDYT? The same applies for the one added to the public ip api.
--
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/pull/1225#pullrequestreview-135651453
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Ignasi Barrera <no...@github.com>.
Closed #1225.
--
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/pull/1225#event-1730748311
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @@ -119,6 +119,16 @@ public void listPublicIPAddresses() {
assertTrue(ipList.size() > 0);
}
+ @Test(groups = "live", dependsOnMethods = "createPublicIPAddress")
+ public void listAllPublicIPAddresses() {
same :)
--
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/pull/1225#discussion_r201353593
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Daniel Estévez <no...@github.com>.
Removed previous solution and added the optional/nullable resourcegroup parameter as solution!
--
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/pull/1225#issuecomment-403970005
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @@ -105,6 +105,12 @@ public void listNetworkInterfaceCards() {
assertTrue(nicList.contains(api().get(nicName)));
}
+ @Test(dependsOnMethods = "createNetworkInterfaceCard")
+ public void listAllNetworkInterfaceCards() {
Good catch, that test could definitely fail.
--
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/pull/1225#discussion_r201353467
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> @SelectJson("value")
@GET
@Fallback(EmptyListOnNotFoundOr404.class)
List<NetworkInterfaceCard> list();
+ @Named("networkinterfacecard:list_all")
+ @Path("/providers/Microsoft.Network/networkInterfaces")
+ @SelectJson("value")
+ @GET
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ List<NetworkInterfaceCard> listAll();
You can declare it nullable and pass a null value there. I'd say that's a better option (for one method that was not needed until now, so we can assume there is no big impact in that low-usage path).
Otherwise, let's go for the global APIs instead of changing the existing APIs that much. The azure provider is no longer in labs and we must keep backward compatibility. If you want to go that path, then you need to `@Deprecate` the methods first till the next release, leave one release for deprecation with a clear upgrade path, then finally remove it in the next one.
--
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/pull/1225#discussion_r201473420
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @SelectJson("value")
@GET
@Fallback(EmptyListOnNotFoundOr404.class)
List<NetworkInterfaceCard> list();
+ @Named("networkinterfacecard:list_all")
+ @Path("/providers/Microsoft.Network/networkInterfaces")
+ @SelectJson("value")
+ @GET
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ List<NetworkInterfaceCard> listAll();
Been thinking about this one... i like *listAll* since the path already has the subscription and requires no resourcegroup, so *listAll* is (obviously?) retrieving all the IPs in subscription.
However this led me to notice a bigger problem: to retrieve all Ips in subscription you're now required to provide a resourcegroupname just to get the reference this API and this makes no sense
https://github.com/danielestevez/jclouds/blob/00b6697d096b54a1f4dc6cbb6a308f91cb0728fc/providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/AzureComputeApi.java#L118
I think we should remove the required *@PathParam("resourcegroup") String resourcegroup* in this case, but it looks like a big change... see any problem with this?
Same applies to the publicIpAddressApi
--
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/pull/1225#discussion_r201356587
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @SelectJson("value")
@GET
@Fallback(EmptyListOnNotFoundOr404.class)
List<NetworkInterfaceCard> list();
+ @Named("networkinterfacecard:list_all")
+ @Path("/providers/Microsoft.Network/networkInterfaces")
+ @SelectJson("value")
+ @GET
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ List<NetworkInterfaceCard> listAll();
But in that 3rd option, the user is forced to pass a resource group he doesn't know anything about (it could even not exist) and that's very confusing
I added 2 commits with the changes to those APIs but i guess that breaks compatibility with clients and won't be accepted, right? I believe, however, it's the best way to go
--
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/pull/1225#discussion_r201461297
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Daniel Estévez <no...@github.com>.
@danielestevez pushed 3 commits.
ebf4a1c Addresses comments on tests
4ce685a Removes resourceGroup parameter from NetworkInterfaceCardApi
ca677c6 Removes resourceGroup parameter from PublicIPAddressApi
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1225/files/cb31ce999e3fc6ba20bff90e4b27c17a88aff5da..ca677c6844ded096d17908f6f8ec1493f51068ed
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> + String path = String
+ .format("/subscriptions/%s/providers/Microsoft.Network/networkInterfaces?%s", subscriptionid, apiVersion);
+
+ assertSent(server, "GET", path);
+ assertTrue(nicList.size() == 3);
+ assertTrue(nicList.get(0).properties().ipConfigurations().size() > 0);
+ assertEquals(nicList.get(0).properties().ipConfigurations().get(0).properties().privateIPAllocationMethod(),
+ "Dynamic");
+ assertTrue(nicList.get(1).properties().ipConfigurations().size() > 0);
+ assertEquals(nicList.get(1).properties().ipConfigurations().get(0).properties().privateIPAllocationMethod(),
+ "Static");
+ assertTrue(nicList.get(2).properties().ipConfigurations().size() > 0);
+ assertNotEquals(IdReference.extractResourceGroup(nicList.get(2).id()), resourcegroup);
+ assertEquals(nicList.get(2).properties().ipConfigurations().get(0).properties().privateIPAllocationMethod(),
+ "Static");
+ }
Forgot that one, 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/pull/1225#discussion_r201353514
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @SelectJson("value")
@GET
@Fallback(EmptyListOnNotFoundOr404.class)
List<NetworkInterfaceCard> list();
+ @Named("networkinterfacecard:list_all")
+ @Path("/providers/Microsoft.Network/networkInterfaces")
+ @SelectJson("value")
+ @GET
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ List<NetworkInterfaceCard> listAll();
The Nullable solution seems the less intrusive solution for this case, i guess. Will do
--
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/pull/1225#discussion_r201480750
Re: [jclouds/jclouds] Adds listall methods to NetworkInterfaceCardApi
and PublicIPAddressApi (#1225)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> @SelectJson("value")
@GET
@Fallback(EmptyListOnNotFoundOr404.class)
List<NetworkInterfaceCard> list();
+ @Named("networkinterfacecard:list_all")
+ @Path("/providers/Microsoft.Network/networkInterfaces")
+ @SelectJson("value")
+ @GET
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ List<NetworkInterfaceCard> listAll();
Yes, that was my point. To get the API you have to pass the resource group, then the method name is confusing.
I see three options:
* Create another api, say `GlobalNetworkApi` with an accessor that does not require the resource group, and put there the method to list all. The con is that you need one specific API for every *listAll* method you want to expose.
* Create a `SubscriptionApi` and put there all methods that access subscription-wide resources.
* Since it is just one method that causes confusion, it is not a big deal to rename it to make it clear what it does.
As per my comment, I'd go for the third option, as users already know that all network operations are exposed in the network API, and for me it makes more sense, even though that resource group is not going to be used by this concrete method.
--
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/pull/1225#discussion_r201408746