Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r88660968
--- Diff:
locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java
---
@@ -0,0 +1,303 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+/**
+ * Utility for manipulating security groups.
+ *
+ * For convenience, constructors take an argument of a {@link
ComputeService} and extract from it the
+ * information needed to edit groups. However, not all compute services
support the security group extension,
+ * so if the editor is being created for a compute service where it is not
known in advance then users
+ * should query {@link #hasServiceSupport()} before calling the methods to
edit security groups.
+ */
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+ public static final java.lang.String JCLOUDS_PREFIX = "jclouds#";
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor for editor that will retry operations upon exceptions.
+ * @param location JClouds location where security groups will be
managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can
retry a request (e.g. to add a security group
+ * or a rule) if the attempted operation throws a Throwable.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location,
ComputeService computeService, Predicate predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId =
this.computeService.getContext().unwrap().getId();
--- End diff --
Have given this and the previous comment further thought and discussion
with @neykov. The choice here seems to be between
1. going "higher level" and trying to pass in a Brooklyn class to the
constructor, from which it could extract the location and compute service.
This would make the API JClouds-unaware, and turn the class into a more widely
re-usable tool for manipulating security-group-like-things however they were
implemented.
2. going "lower level" and leaving it up to the caller to get the required
objects - security group extension and location, with a resultingly simpler API
for this class.
While the former might be nicer to have, it looks like it would require a
bit of rework in the `JcloudsLocationSecurityGroupCustomizer` and/or
`JCloudsLocation` classes, as there are a couple of paths through them that
require this functionality, and these get the two required objects from
different sources - specifically, one path gets the location from a Template,
the other from the actual node's machine location (via
`JcloudsMachineLocation`). This seems too much to take on for the sake of this
PR.
In short, I'll Keep it Simple and go with option 2.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
