Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1735)

Issue 11655043: Fix ensureGroup bug in Openstack provider (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 3 months ago by wallyworld
Modified:
6 years, 3 months ago
Reviewers:
mp+176096, gz, thumper, rog
Visibility:
Public.

Description

Fix ensureGroup bug in Openstack provider It appears not all Openstack providers return a duplicate error if the security group already exists. Rework the business logic to account for this. Also, if a group exists, do not overwrite its rules with any new ones. This brinsg the juju-core behaviour in line with what pyjuju does. https://code.launchpad.net/~wallyworld/juju-core/duplicate-security-group-1189507/+merge/176096 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -4 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M environs/openstack/export_test.go View 1 chunk +4 lines, -0 lines 0 comments Download
M environs/openstack/local_test.go View 2 chunks +37 lines, -0 lines 7 comments Download
M environs/openstack/provider.go View 2 chunks +14 lines, -4 lines 2 comments Download

Messages

Total messages: 5
wallyworld
Please take a look.
6 years, 3 months ago (2013-07-22 00:42:37 UTC) #1
thumper
On 2013/07/22 00:42:37, wallyworld wrote: > Please take a look. LGTM
6 years, 3 months ago (2013-07-22 04:52:50 UTC) #2
gz
LGTM https://codereview.appspot.com/11655043/diff/1/environs/openstack/local_test.go File environs/openstack/local_test.go (right): https://codereview.appspot.com/11655043/diff/1/environs/openstack/local_test.go#newcode483 environs/openstack/local_test.go:483: func (s *localServerSuite) TestEnsureGroup(c *C) { I'm a ...
6 years, 3 months ago (2013-07-22 12:35:50 UTC) #3
rog
LGTM https://codereview.appspot.com/11655043/diff/1/environs/openstack/local_test.go File environs/openstack/local_test.go (right): https://codereview.appspot.com/11655043/diff/1/environs/openstack/local_test.go#newcode483 environs/openstack/local_test.go:483: func (s *localServerSuite) TestEnsureGroup(c *C) { On 2013/07/22 ...
6 years, 3 months ago (2013-07-22 12:48:46 UTC) #4
wallyworld
6 years, 3 months ago (2013-07-23 01:55:31 UTC) #5
https://codereview.appspot.com/11655043/diff/1/environs/openstack/local_test.go
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/11655043/diff/1/environs/openstack/local_test....
environs/openstack/local_test.go:483: func (s *localServerSuite)
TestEnsureGroup(c *C) {
On 2013/07/22 12:48:46, rog wrote:
> On 2013/07/22 12:35:50, gz wrote:
> > I'm a little confused by this test, I guess it relies on the behaviour of
our
> > mock which will otherwise just create a new group if the port details are
> > different?
> 
> a comment might be good.

Done.

https://codereview.appspot.com/11655043/diff/1/environs/openstack/local_test....
environs/openstack/local_test.go:483: func (s *localServerSuite)
TestEnsureGroup(c *C) {
On 2013/07/22 12:35:50, gz wrote:
> I'm a little confused by this test, I guess it relies on the behaviour of our
> mock which will otherwise just create a new group if the port details are
> different?

The business logic of the implementation has been changed to match what was done
in py juju. If a group exists, assume the rules are correct and don't reapply
them.

https://codereview.appspot.com/11655043/diff/1/environs/openstack/local_test....
environs/openstack/local_test.go:496: c.Assert(*group.Rules[0].ToPort, Equals,
22)
On 2013/07/22 12:48:46, rog wrote:
> On 2013/07/22 12:35:50, gz wrote:
> > Middle two Assert would be better as Check?
> 
> agreed.
> perhaps make all except the first one into Checks.

Done.

https://codereview.appspot.com/11655043/diff/1/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/11655043/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:1032: // First attempt to lookup an existing
group by name.
On 2013/07/22 12:48:46, rog wrote:
> s/lookup/look up/

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b