jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
Date Fri, 02 Feb 2018 11:23:39 GMT
nacx commented on this pull request.



> -   @Provides
-   @Singleton
-   @Named("SECURITYGROUP_PRESENT")
-   protected final Predicate<AtomicReference<RegionAndName>> securityGroupEventualConsistencyDelay(
-            FindSecurityGroupWithNameAndReturnTrue in,
-            @Named(TIMEOUT_SECURITYGROUP_PRESENT) long msDelay) {
-      return retry(in, msDelay, 100L, MILLISECONDS);
-   }
+//   @Provides
+//   @Singleton
+//   @Named("SECURITYGROUP_PRESENT")
+//   protected final Predicate<AtomicReference<RegionAndName>> securityGroupEventualConsistencyDelay(
+//            FindSecurityGroupWithNameAndReturnTrue in,
+//            @Named(TIMEOUT_SECURITYGROUP_PRESENT) long msDelay) {
+//      return retry(in, msDelay, 100L, MILLISECONDS);
+//   }

Remove the commented code.

> +       * builder.ipPermissions(filter(transform(group.getRules(), new
+       * Function<SecurityGroupRule, IpPermission>() {
+       * 
+       * @Override public IpPermission apply(SecurityGroupRule from) { if (from.g() ==
+       * RuleDirection.EGRESS) return null; IpPermission.Builder builder =
+       * IpPermission.builder(); if (from.getIpProtocol() != null) {
+       * builder.ipProtocol(from.getIpProtocol()); } else {
+       * builder.ipProtocol(IpProtocol.TCP); } // if (from.getFromPort() != null)
+       * builder.fromPort(from.getFromPort()); // if (from.getToPort() != null)
+       * builder.toPort(from.getToPort()); if (from.getRemoteGroupId() != null) {
+       * builder.groupId(regionId + "/" + from.getGroup()); } else if
+       * (from.getRemoteIpPrefix() != null){
+       * builder.cidrBlock(from.getRemoteIpPrefix()); }
+       * 
+       * return builder.build(); } }), Predicates.notNull())); }
+       */

Remove the commented code.

> +       * the same tenant-id-and-name as that of ruleGroup then it is not possible with
+       * the Nova /v2/12345/os-security-groups API to know which group is intended, as
+       * the only information it returns about referred groups is the tenant id and
+       * name: "group": { "tenant_id": "a0ade3ca76784719845363979dc1014e", "name":
+       * "jclouds-qa-scheduler-docker-entity" }, Rather than pick one group at random
+       * and risk using the wrong group, here we fall back to the least-worst
+       * option(?) and refuse to add any rule.
+       * 
+       * See https://issues.apache.org/jira/browse/JCLOUDS-1234.
+       * 
+       * if (referredGroup.size() != 1) { logger.
+       * warn("Ambiguous group %s used in security rule, refusing to add it to %s (%s)"
+       * , ruleGroup, owningGroup.getName(), owningGroup.getId()); return null; }
+       * builder.groupId(group.getRegion() + "/" +
+       * Iterables.getOnlyElement(referredGroup).getId()); }
+       */

Remove the commented code.

>           }
-         templateOptions.securityGroups(securityGroupName);
-         tagsBuilder.add(String.format("%s-%s", JCLOUDS_SG_PREFIX, securityGroupInRegion.getSecurityGroup().getId()));
+
+         if (securityGroup == null) {

At this point this is always null. Remove the dedundant `if` and declare the variable here.

-- 
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/1175#pullrequestreview-93589738
Mime
View raw message