Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
Closed #1006. -- 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/1006#event-1096914031
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
Superseded by https://github.com/jclouds/jclouds/pull/1106. -- 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/1006#issuecomment-303951835
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
@utkarsh-devops This PR has been quiet for some time. Feel free to step in and help with the remaining bits! -- 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/1006#issuecomment-302005219
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
We are waiting for the release of this feature. please expedite. Tx -- 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/1006#issuecomment-301759282
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
@nacx: argh!! the notifications for your reply got lost in my inbox and I did not see them. sorry for the delay! i'll get to it asap. -- 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/1006#issuecomment-254281357
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
They take some time, so I'd recommend that you just run the class where you add the new tests :) -- 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/1006#issuecomment-249135944
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
> How do I configure my local machine to run the live tests The better options is to download your credentials as a JSON file. Once you have them, you can run all live tests as follows: mvn clean install -Plive -Dtest.google-cloud.json-key=/path/to/your/credentials.json Or if you want to run just one single test class: mvn integration-test -Plive -Dtest.google-cloud.json-key=/path/to/your/credentials.json -Dtest=TheClassTestToRun -- 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/1006#issuecomment-249135806
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
nacx commented on this pull request.
>
/**
* This must be within the range specified by IPv4Range, and is typically
the first usable address in that range.
* If not specified, the default value is the first usable address in
IPv4Range.
*/
@Nullable public abstract String gatewayIPv4();
- @SerializedNames({ "id", "creationTimestamp", "selfLink", "name",
"description", "IPv4Range", "gatewayIPv4" })
+ @Nullable public abstract List subnetworks();
+
+ public static Network create(String id, Date creationTimestamp, URI
selfLink, String name, String description, String rangeIPv4,
+String gatewayIPv4) {
+ return new AutoValue_Network(id, creationTimestamp, selfLink, name,
NetworkType.LegacyNetwork, description,
+ rangeIPv4, gatewayIPv4, null);
+ }
This will be merged and released to 2.0, so we can expect some breaking
changes. Trivial ones such as this one shouldn't worry us.
An option to make this more update-friendly for future releases would be to
provide a builder with AutoValue.Builder. In any case, I think we can just
break this in 2.0 and keep jsut one factory method (plus the builder if you
want). Same applies for other similar comments on this.
--
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/1006
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
@nacx: How do I configure my local machine to run the live tests? I found some documentation that alludes to adding to the m2 settings, but I do not know what that means :-) That's why I did not write the live tests (yet). I was going to do another PR with that, but if you can help me with setting that up, so I can test it, I shall do it right away. -- 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/1006#issuecomment-248361338
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
nelsonjr commented on this pull request.
>@SerializedNames({ "network", "accessConfigs" })
static NetworkInterface create(URI network, List
accessConfigs) {
- return new AutoValue_NewInstance_NetworkInterface(network,
accessConfigs);
+ return new AutoValue_NewInstance_NetworkInterface(network, null,
accessConfigs);
+ }
+
+ @SerializedNames({ "network", "subnetwork", "accessConfigs" })
+ static NetworkInterface create(URI network, URI subnetwork,
List accessConfigs) {
+ return new AutoValue_NewInstance_NetworkInterface(network,
subnetwork, accessConfigs);
[ditto]
--
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/1006
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
nelsonjr commented on this pull request.
>
/**
* This must be within the range specified by IPv4Range, and is typically
the first usable address in that range.
* If not specified, the default value is the first usable address in
IPv4Range.
*/
@Nullable public abstract String gatewayIPv4();
- @SerializedNames({ "id", "creationTimestamp", "selfLink", "name",
"description", "IPv4Range", "gatewayIPv4" })
+ @Nullable public abstract List subnetworks();
+
+ public static Network create(String id, Date creationTimestamp, URI
selfLink, String name, String description, String rangeIPv4,
+String gatewayIPv4) {
+ return new AutoValue_Network(id, creationTimestamp, selfLink, name,
NetworkType.LegacyNetwork, description,
+ rangeIPv4, gatewayIPv4, null);
+ }
I thought this would cause problems if the customer was creating the network in
their code using this constructor. That's why I moved the automatic to the more
generic but I kept the original one in place.
--
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/1006
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
nelsonjr commented on this pull request. > @@ -32,6 +35,7 @@ private List serviceAccounts; private String bootDiskType; private boolean preemptible = false; + private Set subnetworks; [explained in previous comment] TL;DR: subnetworks = networks, multiple. -- 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/1006
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
nelsonjr commented on this pull request.
> +@Singleton
+public class SubnetworkLoader extends CacheLoader {
+ @Resource
+ protected Logger logger = Logger.NULL;
+
+ private final Resources resources;
+
+ @Inject
+ SubnetworkLoader(Resources resources) {
+ this.resources = resources;
+ }
+
+ @Override
+ public Subnetwork load(URI key) throws ExecutionException {
+ try {
+ return resources.subnetwork(key);
Like networks it requires an API call to gather its details and may drive
customers over quota/cost. I'm modeling subnetworks just like networks, because
in practical sense they are.
(I almost made subnetworks deriving from network :-) but the different settings
deterred me from doing that, and extracting an interface at this point would
generate a breaking change that it is not worth)
--
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/1006
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
nelsonjr commented on this pull request. > @@ -136,11 +136,17 @@ URI network = URI.create(networks.next()); assert !networks.hasNext() : "Error: Options should specify only one network"; + Iterator subnetworks = options.getSubnetworks().iterator(); + + URI subnetwork = subnetworks.hasNext() ? URI.create(subnetworks.next()) : null; + assert !subnetworks.hasNext() : "Error: Options should specify only one subnetwork"; I made a collection like networks because like networks it can be specified more than once. I added the blocker to match networks behavior though, but I'll create a separate PR to remove both and bring multiple interfaces (direct or subnetworked) to the module. -- 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/1006
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
Thanks @nelsonjr! This looks great. Just a couple comments apart from the inline ones: * Add mock tests for the methods added to the aggregated list api. * Add live tests for the subnetworks api and the new methods in the aggregated list api. These ones are specially important, since they warn us when things change in the provider. -- 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/1006#issuecomment-245662717
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
> @@ -126,7 +124,7 @@ Operation createInIPv4Range(@PayloadParam("name") String
> networkName,
> @Transform(NetworkPages.class)
> Iterator> list(ListOptions options);
>
> - static final class NetworkPages extends BaseToIteratorOfListPage NetworkPages> {
> + final class NetworkPages extends BaseToIteratorOfListPage NetworkPages> {
What is the purpose of removing the static modifier?
--
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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78043212
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
> @@ -82,6 +83,10 @@
> @Path("/stop")
> Operation stopInstance(@EndpointParam URI selfLink);
>
> + @Named("Subnetworks:get")
> + @GET
> + @Fallback(NullOnNotFoundOr404.class) @Nullable Subnetwork
> subnetwork(@EndpointParam URI selfLink);
This method is not used. Remove it?
--
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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78044528
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
> + this.resources = resources;
> + }
> +
> + @Override
> + public Subnetwork load(URI key) throws ExecutionException {
> + try {
> + return resources.subnetwork(key);
> + } catch (Exception e) {
> + throw new ExecutionException(message(key, e), e);
> + }
> + }
> +
> + public static String message(URI key, Exception e) {
> + return String.format("could not find image for disk %s: %s",
> key.toString(), e.getMessage());
> + }
> +}
Looking in more detail, it looks like this cache is not used. Better remove it.
--
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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78044468
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
> + assertSent(server, "DELETE",
> "/projects/party/regions/someregion/subnetworks/jclouds-test");
> + }
> +
> + public void list() throws Exception {
> + server.enqueue(jsonResponse("/subnetwork_list.json"));
> +
> + assertEquals(subnetworkApi().list().next(), new
> ParseSubnetworkListTest().expected(url("/projects")));
> + assertSent(server, "GET",
> "/projects/party/regions/someregion/subnetworks");
> + }
> +
> + public void list_empty() throws Exception {
> + server.enqueue(jsonResponse("/list_empty.json"));
> +
> + assertFalse(subnetworkApi().list().hasNext());
> + assertSent(server, "GET",
> "/projects/party/regions/someregion/subnetworks");
> + }
Add mock tests for the `listPage` methods.
--
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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78043614
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
>
> /**
> * This must be within the range specified by IPv4Range, and is typically
> the first usable address in that range.
> * If not specified, the default value is the first usable address in
> IPv4Range.
> */
> @Nullable public abstract String gatewayIPv4();
>
> - @SerializedNames({ "id", "creationTimestamp", "selfLink", "name",
> "description", "IPv4Range", "gatewayIPv4" })
> + @Nullable public abstract List subnetworks();
> +
> + public static Network create(String id, Date creationTimestamp, URI
> selfLink, String name, String description, String rangeIPv4,
> +String gatewayIPv4) {
> + return new AutoValue_Network(id, creationTimestamp, selfLink, name,
> NetworkType.LegacyNetwork, description,
> + rangeIPv4, gatewayIPv4, null);
> + }
To avoid errors in serialisation, it is better to provide just one factory
method. Let's remove this one and keep just the new 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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78042181
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
>@SerializedNames({ "network", "accessConfigs" })
>static NetworkInterface create(URI network, List
> accessConfigs) {
> - return new AutoValue_NewInstance_NetworkInterface(network,
> accessConfigs);
> + return new AutoValue_NewInstance_NetworkInterface(network, null,
> accessConfigs);
> + }
> +
> + @SerializedNames({ "network", "subnetwork", "accessConfigs" })
> + static NetworkInterface create(URI network, URI subnetwork,
> List accessConfigs) {
> + return new AutoValue_NewInstance_NetworkInterface(network,
> subnetwork, accessConfigs);
Same here. Keep just one static factory 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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78042253
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
> @@ -32,6 +35,7 @@ > private List serviceAccounts; > private String bootDiskType; > private boolean preemptible = false; > + private Set subnetworks; Change to just `URI subnetwork`, as mentioned above? -- 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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78041854
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
> +
> + @Inject
> + SubnetworkLoader(Resources resources) {
> + this.resources = resources;
> + }
> +
> + @Override
> + public Subnetwork load(URI key) throws ExecutionException {
> + try {
> + return resources.subnetwork(key);
> + } catch (Exception e) {
> + throw new ExecutionException(message(key, e), e);
> + }
> + }
> +
> + public static String message(URI key, Exception e) {
Make this private
--
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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78041781
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
> +@Singleton
> +public class SubnetworkLoader extends CacheLoader {
> + @Resource
> + protected Logger logger = Logger.NULL;
> +
> + private final Resources resources;
> +
> + @Inject
> + SubnetworkLoader(Resources resources) {
> + this.resources = resources;
> + }
> +
> + @Override
> + public Subnetwork load(URI key) throws ExecutionException {
> + try {
> + return resources.subnetwork(key);
Is this such a freqüent/expensive operation that requires a cache?
--
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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78041673
Re: [jclouds/jclouds] JCLOUDS-1122: Support subnetworks definitions in Google Compute. (#1006)
> @@ -136,11 +136,17 @@ >URI network = URI.create(networks.next()); >assert !networks.hasNext() : "Error: Options should specify only one > network"; > > + Iterator subnetworks = options.getSubnetworks().iterator(); > + > + URI subnetwork = subnetworks.hasNext() ? > URI.create(subnetworks.next()) : null; > + assert !subnetworks.hasNext() : "Error: Options should specify only > one subnetwork"; Since this is an option specific to google (unlike "networks" that is in the jclouds portable interface), make it just an URI instead of a collection, to avoid unnecessary configuration issues. -- 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/1006/files/0c51ed39650e9cab341136716a9d9732c63f7dd0#r78041485
