thanks @nacx for your help and review
--
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#issuecomment-362618718
merging 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/1175#issuecomment-362617035
Closed #1175.
--
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#event-1455308130
merged at
[master](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;h=09936b5)
--
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#issuecomment-362618900
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/1175#pullrequestreview-93641279
nacx commented on this pull request.
> - @Provides
- @Singleton
- @Named("SECURITYGROUP_PRESENT")
- protected final Predicate
securityGroupEventualConsistencyDelay(
-FindSecurityGroupWithNameAndReturnTrue in,
-
@andreaturli pushed 1 commit.
ff4a725 simplify securitygroup cache
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1175/files/753638f986a376e6abd94ccc57ea35bad91a79af..ff4a7257f537a28788cfdf3d0956808a4115c4c2
@andreaturli pushed 1 commit.
753638f fix listSecurityGroupsForNode
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1175/files/4e0b06b2510001ccd9a83f3166989d6119255f46..753638f986a376e6abd94ccc57ea35bad91a79af
nacx commented on this pull request.
> + return ImmutableSet.of();
+ }
+ return
getSecurityGroupApi(region).listSecurityGroups().concat().transform(neutronSecurityGroupToSecurityGroup.create(location)).toSet();
+ }
+
+ @Override
+ public Set
andreaturli commented on this pull request.
> + return ImmutableSet.of();
+ }
+ return
getSecurityGroupApi(region).listSecurityGroups().concat().transform(neutronSecurityGroupToSecurityGroup.create(location)).toSet();
+ }
+
+ @Override
+ public Set
andreaturli commented on this pull request.
> + return ImmutableSet.of();
+ }
+ return
getSecurityGroupApi(region).listSecurityGroups().concat().transform(neutronSecurityGroupToSecurityGroup.create(location)).toSet();
+ }
+
+ @Override
+ public Set
@nacx here's the result of `NeutronSecurityGroupExtensionLiveTest` against a
provider with keystone v3 and Neutron support
```
---
T E S T S
---
Running
nacx requested changes on this pull request.
I've debugged the failure and it turns to be a test concurrency issue with the
two tests that verify the behavior of the SG cache. If you apply this patch to
make sure both tests don't use the same security group, tests should pass:
```diff
diff
here's my live tests results:
- for `NovaSecurityGroupExtension`
```
---
T E S T S
---
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=256m;
support was removed in 8.0
@andreaturli pushed 1 commit.
4e0b06b fix removal from security group cache
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
nacx commented on this pull request.
Just one comment. Apart from that looks good.
Could you share the results of the live tests for the Nova and Neutron SG
extensions?
> + if (region == null) {
+ return ImmutableSet.of();
+ }
+ return
rebuild please
--
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#issuecomment-359433463
@nacx I think I've addressed most of your comments and now the duplication of
`Find/Create` SecurityGroup is gone.
I still have some expected tests to fix but please let me know if the code is
more readable now. Thanks
--
You are receiving this because you are subscribed to this thread.
@andreaturli pushed 1 commit.
0bdbdbf addressing Nacx comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1175/files/7846a2ddf32ed07e6fc394a43d954fc6fe039d28..0bdbdbf29214504f4e676e0afd6fb43f83275e4e
@nacx thanks for the review
Main reason I have 2 couples of `Create/FindSecurityGroup` now (I want to
remove them before merging this PR) is because of `NovaSecurityGroupExtension`
and `NeutronSecurityGroupExtension`
`NovaSecurityGroupExtension` needs to use `NovaApi` to create security groups,
nacx requested changes on this pull request.
Thanks @andreaturli! Looks like a great starting point.
Apart from the comments I've made I found it difficult to understand the
duplicated "Create/FindSecurityGroup" classes. For a proper review, could you
please:
* Explain what is each new class,
cc @nacx
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1175
-- Commit Summary --
* [JCLOUDS-1377] add support for injectable Neutron Context into Nova
-- File Changes --
M apis/openstack-nova/pom.xml (5)
M
apis/openstack
22 matches
Mail list logo
