Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

openstack modifications #2462

Merged
merged 1 commit into from Dec 10, 2013

Conversation

Projects
None yet
5 participants
Contributor

ehowe commented Dec 7, 2013

  • add model and collection for security group rules
  • add mock data for networks
  • returns address in create_server mock
  • proper security group rule mocks
  • proper security group mocks

Coverage Status

Coverage remained the same when pulling 91690b3eb78b8da5022c9258c3ae8073ecb72a46 on engineyard:openstack into 216cf26 on fog:master.

Coverage Status

Coverage remained the same when pulling 91690b3eb78b8da5022c9258c3ae8073ecb72a46 on engineyard:openstack into 216cf26 on fog:master.

Owner

geemus commented Dec 8, 2013

@krames - could you review? Thanks!

@krames krames and 1 other commented on an outdated diff Dec 9, 2013

lib/fog/openstack/models/compute/security_group_rules.rb
@@ -0,0 +1,21 @@
+require 'fog/core/collection'
+require 'fog/openstack/models/compute/security_group_rule'
+
+module Fog
+ module Compute
+ class OpenStack
+ class SecurityGroupRules < Fog::Collection
+
+ model Fog::Compute::OpenStack::SecurityGroupRule
+
+ def get(security_group_rule_id)
+ if security_group_rule_id
+ new(service.get_security_group_rule(security_group_rule_id).body['security_group_rule'])
@krames

krames Dec 9, 2013

Owner

Can we break this into two lines? It should make it easier to find issues if we ever get a stack trace.

@thommahoney

thommahoney Dec 10, 2013

Contributor

Changing it to this:

  body = service.get_security_group_rule(security_group_rule_id).body
  new(body['security_group_rule'])

@krames krames and 1 other commented on an outdated diff Dec 9, 2013

lib/fog/openstack/models/compute/security_group.rb
@@ -13,6 +13,9 @@ class SecurityGroup < Fog::Model
attribute :rules
attribute :tenant_id
+ def security_group_rules
+ Fog::Compute::OpenStack::SecurityGroupRules.load(rules)
@krames

krames Dec 9, 2013

Owner

Would users ever use the rules attribute directly? If not, what do you think about adding deprecation warnings if users try to access this attribute and changing this line to

Fog::Compute::OpenStack::SecurityGroupRules.load(attributes[:rules])

@thommahoney

thommahoney Dec 10, 2013

Contributor

Will do. Have a look at the next commit (I'll comment when I make the commit).

Owner

krames commented Dec 9, 2013

@ehowe Thanks for the PR!

Overall it looks great, but I did have a couple comments. In addition to those, can I get you to write tests for get_security_group_rule request and the model security_group_rule and collection for security_group_rules?

Here are a couple examples to get you started:

request test => https://github.com/fog/fog/blob/master/tests/rackspace/requests/compute_v2/keypair_tests.rb
model test => https://github.com/fog/fog/blob/master/tests/rackspace/models/monitoring/agent_token_tests.rb
collection test => https://github.com/fog/fog/blob/master/tests/rackspace/models/monitoring/agent_tokens_tests.rb

Let me know if you need any help getting started with our testing framework shindo.

@lanej Thom Mahoney & Eugene Howe openstack modifications
* add model and collection for security group rules
* add mock data for networks
* returns address in create_server mock
* proper security group rule mocks
* proper security group mocks
0192e53

Coverage Status

Coverage increased (+0.59%) when pulling 0192e53 on engineyard:openstack into 8d1062b on fog:master.

Contributor

thommahoney commented Dec 10, 2013

@krames what do you think?

Owner

krames commented Dec 10, 2013

@thommahoney LGTM! :shipit:

Thanks again for the PR!

@krames krames pushed a commit that referenced this pull request Dec 10, 2013

Kyle Rames Merge pull request #2462 from engineyard/openstack
openstack modifications
5346176

@krames krames merged commit 5346176 into fog:master Dec 10, 2013

1 check passed

default The Travis CI build passed
Details
Owner

geemus commented Dec 11, 2013

Thanks!

On Tue, Dec 10, 2013 at 8:19 AM, Kyle Rames notifications@github.comwrote:

Merged #2462 #2462.


Reply to this email directly or view it on GitHubhttps://github.com/fog/fog/pull/2462
.

Contributor

thommahoney commented on 0192e53 Dec 16, 2013

@burns you are correct. We're using string keys for the security group mock data now. So that seed data does need to change.

Contributor

thommahoney replied Dec 16, 2013

@burns master seems to be green. Let me know if you want me to look into a specific failure. https://travis-ci.org/fog/fog/builds/15552477

Owner

geemus replied Dec 17, 2013

Yeah, any insight would be awesome (I was just trying to look in to it and nothing was jumping out at me as obvious in terms of why it would sometimes be failing on us).

Owner

geemus replied Dec 17, 2013

Just did a bit of work around it here: b056df5

Not sure it will fix it (since I'm not sure why it was breaking), but it at least streamlines and removes some of the failure vectors.

Owner

geemus replied Dec 17, 2013

Looks like it might be fixed, fwiw. Now a different error is breaking the build anyway. Will continue to monitor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment