You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Mathieu Tortuyaux <no...@github.com> on 2018/08/24 16:42:36 UTC
[jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
This is the [jira](https://issues.apache.org/jira/browse/JCLOUDS-1415). I've wrote one test for the firewall loader, but I think it's not enough.
And I've tried to make a Minimum Viable Change in order to don't have a lot of new files and things to review.
Thanks for your review !
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1236
-- Commit Summary --
* feat(firewall): add loader class
* feat(resource): add GET api to get firewall by uri
* feat(node/create): use firewall loader
* feat(compute/service): create firewall provider
* style(resources): use multilines import
* perf(node/creation): remove comments
* test(firewall/loader): add one test to handle not found firewall
* style(node/creation): fix checkstyle violation
* chore(firewall/test): add missing apache licence
* perf(firewall/loader): remove space between package and licence
* feat(adapter): try to get first the resource by URI
* feat(node/creation): check on the full URI
-- File Changes --
M providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapter.java (10)
M providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/config/GoogleComputeEngineServiceContextModule.java (13)
M providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/Resources.java (6)
A providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/loaders/FirewallLoader.java (53)
M providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/strategy/CreateNodesWithGroupEncodedIntoNameThenAddToSet.java (99)
A providers/google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/compute/loaders/FirewallLoaderTest.java (42)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1236.patch
https://github.com/jclouds/jclouds/pull/1236.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/1236
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Andrew Gaul <no...@github.com>.
Closed #1236.
--
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/1236#event-3586273974
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Mathieu Tortuyaux <no...@github.com>.
@tormath1 pushed 4 commits.
211beb8 perf(node/creation): remove if statement
a0c4bf1 style(context): fix missing indentation
6169621 style(resources): fix spaces in comment
59ebfc1 style(fw/loader): fix indentation from 4 to 3
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1236/files/51801fc0fdfd67a2efda5ee359b104c5ba9fced2..59ebfc1479e3e5285be17369d9cb977202a93f80
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Mathieu Tortuyaux <no...@github.com>.
tormath1 commented on this pull request.
> checkArgument(!iterator.hasNext(),
"Error: Please specify only one network/subnetwork in TemplateOptions when using GCE.");
}
String region = ZONE == location.getScope() ? location.getParent().getId() : location.getId();
- Optional<Subnetwork> subnet = subnetworksMap.getUnchecked(fromRegionAndName(region, networkName));
+ Optional<Subnetwork> subnet;
+
+ subnet = isFullURI ? Optional.fromNullable(resources.subnetwork(URI.create(net))) : subnetworksMap.getUnchecked(fromRegionAndName(region, networkName));
Yes. I was about thinking about something like that. So we have a plan!
--
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/1236#discussion_r214322842
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Mathieu Tortuyaux <no...@github.com>.
Hi @gaul , I gave up this contribution for this time... So I suppose we could close this PR for this time. :disappointed:
--
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/1236#issuecomment-458433857
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Mathieu Tortuyaux <no...@github.com>.
tormath1 commented on this pull request.
> checkArgument(!iterator.hasNext(),
"Error: Please specify only one network/subnetwork in TemplateOptions when using GCE.");
}
String region = ZONE == location.getScope() ? location.getParent().getId() : location.getId();
- Optional<Subnetwork> subnet = subnetworksMap.getUnchecked(fromRegionAndName(region, networkName));
+ Optional<Subnetwork> subnet;
+
+ subnet = isFullURI ? Optional.fromNullable(resources.subnetwork(URI.create(net))) : subnetworksMap.getUnchecked(fromRegionAndName(region, networkName));
Ok. But a shared VPC is identified by his `host project`, `region` and `name` and a classical subnetwork is only identified by his `region` and `name`, the `project` is given by the service account.
In my opinon, if a user wants to use a shared VPC, it'll have to give the full URI and if he wants to use a classical subnetwork, it'll have to give only the `region` and `name`. In this way, we are not breaking the compatibility.
This is why, I am checking if the given `net` is like `projects/<project>/regions/<region>/subnetworks/<subnetwork>`.
--
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/1236#discussion_r214167148
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Andrew Gaul <no...@github.com>.
@nacx @tormath1 does this pull request need anything to move forward?
--
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/1236#issuecomment-458260849
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
Thanks @tormath1!
> @@ -220,6 +225,14 @@ protected void configure() {
return CacheBuilder.newBuilder().build(in);
}
+
+@Provides
+@Singleton
+protected LoadingCache<URI, Optional<Firewall>> firewallsMap(
+ CacheLoader<URI, Optional<Firewall>> in) {
+ return CacheBuilder.newBuilder().build(in);
+}
[minor] fix indentation
> @@ -96,4 +97,9 @@
@Named("Images:get")
@GET
@Fallback(NullOnNotFoundOr404.class) @Nullable Image image(@EndpointParam URI selfLink);
+
+ /** Returns a firewall by self-link or null if not found*/
[minor] fix leading and trailing spaces in the comment.
> +
+ @Override
+ public Optional<Firewall> load(URI key) throws ExecutionException {
+ try {
+ return Optional.fromNullable(resources.firewall(key));
+ } catch (Exception ex) {
+ throw new ExecutionException(message(key, ex), ex);
+ }
+ }
+
+ public static String message(URI key, Exception ex) {
+ return String.format("could not find firewall %s: %s", key.toString(), ex.getMessage());
+ }
+
+
+}
jclouds uses a 3 space indent. Please format the code accordingly.
>
if (options.getNetworks().isEmpty()) {
networkName = DEFAULT_NETWORK_NAME;
} else {
Iterator<String> iterator = options.getNetworks().iterator();
- networkName = nameFromNetworkString(iterator.next());
+ net = iterator.next();
+ if (net.startsWith("projects")) {
+ isFullURI = true;
+ }
Just `isFullURI = net.startsWith("projects")`
> checkArgument(!iterator.hasNext(),
"Error: Please specify only one network/subnetwork in TemplateOptions when using GCE.");
}
String region = ZONE == location.getScope() ? location.getParent().getId() : location.getId();
- Optional<Subnetwork> subnet = subnetworksMap.getUnchecked(fromRegionAndName(region, networkName));
+ Optional<Subnetwork> subnet;
+
+ subnet = isFullURI ? Optional.fromNullable(resources.subnetwork(URI.create(net))) : subnetworksMap.getUnchecked(fromRegionAndName(region, networkName));
Could this logic be abstracted in the subnet cache? Otherwise, we are catching subnets only if the provided ID is not the full URI, which does not seem right?
This way you save an extra call in the adapter. With the current code, you are calling the `resources` API here and calling it again in the adapter.
--
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/1236#pullrequestreview-150895576
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Andrew Gaul <no...@github.com>.
Please reopen against apache/jclouds if this is still relevant.
--
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/1236#issuecomment-663855817
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> checkArgument(!iterator.hasNext(),
"Error: Please specify only one network/subnetwork in TemplateOptions when using GCE.");
}
String region = ZONE == location.getScope() ? location.getParent().getId() : location.getId();
- Optional<Subnetwork> subnet = subnetworksMap.getUnchecked(fromRegionAndName(region, networkName));
+ Optional<Subnetwork> subnet;
+
+ subnet = isFullURI ? Optional.fromNullable(resources.subnetwork(URI.create(net))) : subnetworksMap.getUnchecked(fromRegionAndName(region, networkName));
Ok. The main issue here is that the cache key is `RegionAndName`. What about creating a different class to be used with these loaders that can cache shared resources, say `RegionAndNameInProject`? In the case of shared resources we can build it from the full URI, and for others we can just get injected the current project (we already have the project bound to the Guice contecxt as `@Named(PROJECT_NAME) String currentProject`) and build it too. This way we can still have backwards compatibility and properly cache all subnets whether they are shared or not, saving unnecessary repeated API calls. 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/pull/1236#discussion_r214312128
Re: [jclouds/jclouds] [JCLOUDS-1415] - Support GCE shared VPC (#1236)
Posted by Mathieu Tortuyaux <no...@github.com>.
@tormath1 pushed 1 commit.
51801fc style(node/creation): use inline if
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1236/files/b9310ab7f9e73709cbdf181163d5dd3c20dec52d..51801fc0fdfd67a2efda5ee359b104c5ba9fced2