Fix ensureGroup bug in Openstack provider

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)

[wallyworld, 2013-07-22 00:42:37]

Please take a look.

[thumper, 2013-07-22 04:52:50]

On 2013/07/22 00:42:37, wallyworld wrote:
> Please take a look.

LGTM

[gz, 2013-07-22 12:35:50]

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....
environs/openstack/local_test.go:483: func (s *localServerSuite)
TestEnsureGroup(c *C) {
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?

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)
Middle two Assert would be better as Check?

[rog, 2013-07-22 12:48:46]

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....
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?

a comment might be good.

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:35:50, gz wrote:
> Middle two Assert would be better as Check?

agreed.
perhaps make all except the first one into Checks.

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.
s/lookup/look up/

[wallyworld, 2013-07-23 01:55:31]

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.