Fixes #3650, #11600 - Compute Resource availability_zones, flavors, security_groups API v2 endpoints #2657

kairel · 2015-09-01T10:04:17Z

ref #3650 and ref#11600

theforeman-bot · 2015-09-01T10:04:18Z

There were the following issues with the commit message:

b97c0b2 must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

dLobatog · 2015-09-01T11:02:27Z

app/views/api/v2/compute_resources/available_availability_zones.rabl

@@ -0,0 +1,5 @@
+collection @available_availability_zones, root_object: false
+
+node :name do |s|


Nitpick, there must exist more descriptive name than 's' 😄

dLobatog · 2015-09-01T11:02:48Z

[test] this triggers our Jenkins job

dLobatog · 2015-09-01T11:03:39Z

test/functional/api/v2/compute_resources_controller_test.rb

@@ -96,6 +96,7 @@ def teardown
    assert_response :not_found
  end

+


Remove this extra line and the other one in line 126, they'll likely cause Rubocop to fail

dLobatog · 2015-09-01T11:28:09Z

Hi @kairel ! Thanks for your patch, it's highly appreciated. Also thanks for the putting the work to write the tests for it, it's close to merge IMO, there are a few minor issues I've commented inline.

If you think your commit is fixing #3650 and #11600, reference it as "Fixes #3650, #11600" on your commit. We use the word 'refs' only when there are more than one commit fixing the same issue. Sorry it's a bit confusing!

I'm on the fence about the available_ prefixed to all fog methods here. I'd argue the 'available' part is something we can assume, but I understand why you added it (because it's everywhere already). If others, @isratrade, @ohadlevy, @domcleal are OK with it, I'd say let's drop the prefix for all new methods like these ones & drop it everywhere for API v3.

Thanks again, let us know on #theforeman-dev on Freenode IRC if you have any questions!

theforeman-bot · 2015-09-01T13:17:10Z

There were the following issues with the commit message:

b97c0b2 must be in the format fixes #redmine_number - brief description.
fe22540 must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

dLobatog · 2015-09-01T14:09:19Z

[test]

dLobatog · 2015-09-02T08:57:59Z

@kairel There are a few tests failing & rubocop. Could you take a look at them and update the PR? To see what failed you can just click on Details on the PR status box.

Also we normally take changes in one commit, so if you could git rebase HEAD~2 your branch that'd help. Thanks again!

dLobatog · 2015-09-02T09:53:31Z

app/models/compute_resources/foreman/model/ec2.rb

@@ -71,6 +71,18 @@ def flavors
      client.flavors
    end

+    def available_availability_zones


To avoid making these kind of wrappers, you can write at the beginning of the file, bellow the other delegate:

delegate :security_groups, :flavors, :zones, :to => :self, :prefix => 'available'

available_availability_zones is a bit redundant plus we call them zones on Foreman to be more generic

theforeman-bot · 2015-09-02T13:10:15Z

There were the following issues with the commit message:

38e5e8c must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

dLobatog · 2015-09-03T06:57:40Z

[test] - this allows our Jenkins CI to pick up this PR and test it

theforeman-bot · 2015-09-03T08:24:46Z

There were the following issues with the commit message:

e31b90f must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

kairel · 2015-09-04T12:22:47Z

i must be something for test ?

domcleal · 2015-09-04T12:25:17Z

Sorry! [test] will re-run it since the change.

kairel · 2015-09-07T08:34:06Z

Hi,

All checks have passed, i need to change something ?

domcleal · 2015-09-07T08:41:22Z

It's waiting on a review at the moment, but all the bot is saying that the commit message isn't quite right - there's a missing "#" before the second ticket number.

dLobatog · 2015-09-10T13:31:29Z

app/models/compute_resource.rb

@@ -199,6 +199,10 @@ def supports_update?
    false
  end

+  def available_availability_zones


I think you forgot to rename this to available_zones

dLobatog · 2015-09-10T13:33:41Z

Nice job @kairel ! 2 very minor things and we're done:

The commit title lacks a second # before 11600, it should contain fixes #3650, #11600
See inline comment on compute_resource, there's still a method available_availability_zones

Thank you!

theforeman-bot · 2015-09-11T08:30:08Z

There were the following issues with the commit message:

f5b2e2e must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

…flavors, security_groups API v2 endpoints

dLobatog · 2015-09-11T20:23:00Z

[test]

dLobatog · 2015-09-11T22:06:33Z

Merged as 45c1a85, thanks @kairel!

theforeman-bot added Waiting on contributor Not yet reviewed Needs testing labels Sep 1, 2015

dLobatog reviewed Sep 1, 2015
View reviewed changes

dLobatog removed Needs testing Not yet reviewed labels Sep 1, 2015

dLobatog changed the title ~~ref #3650 adding list values compute resources~~ Fixes #3650, 11600 - Compute Resource availability_zones, flavors, security_groups API v2 endpoints Sep 1, 2015

theforeman-bot added Needs re-review Needs testing Waiting on contributor and removed Waiting on contributor labels Sep 1, 2015

dLobatog reviewed Sep 2, 2015
View reviewed changes

dLobatog removed Needs re-review Needs testing labels Sep 2, 2015

kairel force-pushed the 3650-add_values_compute_resources branch from fe22540 to 38e5e8c Compare September 2, 2015 13:10

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Sep 2, 2015

dLobatog removed the Waiting on contributor label Sep 3, 2015

kairel force-pushed the 3650-add_values_compute_resources branch from cc72486 to e31b90f Compare September 3, 2015 08:24

theforeman-bot added the Waiting on contributor label Sep 3, 2015

kairel changed the title ~~Fixes #3650, 11600 - Compute Resource availability_zones, flavors, security_groups API v2 endpoints~~ Fixes #3650, #11600 - Compute Resource availability_zones, flavors, security_groups API v2 endpoints Sep 7, 2015

domcleal removed the Waiting on contributor label Sep 10, 2015

dLobatog reviewed Sep 10, 2015
View reviewed changes

dLobatog added Waiting on contributor and removed Needs re-review Needs testing labels Sep 10, 2015

kairel force-pushed the 3650-add_values_compute_resources branch from e31b90f to f5b2e2e Compare September 11, 2015 08:30

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Sep 11, 2015

theforeman-bot added the Waiting on contributor label Sep 11, 2015

Fixes theforeman#3650, #11600 - Compute Resource availability_zones, …

45c1a85

…flavors, security_groups API v2 endpoints

kairel force-pushed the 3650-add_values_compute_resources branch from f5b2e2e to 45c1a85 Compare September 11, 2015 08:38

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Sep 11, 2015

dLobatog closed this Sep 11, 2015

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #3650, #11600 - Compute Resource availability_zones, flavors, security_groups API v2 endpoints #2657

Fixes #3650, #11600 - Compute Resource availability_zones, flavors, security_groups API v2 endpoints #2657

kairel commented Sep 1, 2015

theforeman-bot commented Sep 1, 2015

dLobatog Sep 1, 2015

dLobatog commented Sep 1, 2015

dLobatog Sep 1, 2015

dLobatog commented Sep 1, 2015

theforeman-bot commented Sep 1, 2015

dLobatog commented Sep 1, 2015

dLobatog commented Sep 2, 2015

dLobatog Sep 2, 2015

theforeman-bot commented Sep 2, 2015

dLobatog commented Sep 3, 2015

theforeman-bot commented Sep 3, 2015

kairel commented Sep 4, 2015

domcleal commented Sep 4, 2015

kairel commented Sep 7, 2015

domcleal commented Sep 7, 2015

dLobatog Sep 10, 2015

dLobatog commented Sep 10, 2015

theforeman-bot commented Sep 11, 2015

dLobatog commented Sep 11, 2015

dLobatog commented Sep 11, 2015

		@@ -0,0 +1,5 @@
		collection @available_availability_zones, root_object: false

		node :name do \|s\|

Fixes #3650, #11600 - Compute Resource availability_zones, flavors, security_groups API v2 endpoints #2657

Fixes #3650, #11600 - Compute Resource availability_zones, flavors, security_groups API v2 endpoints #2657

Conversation

kairel commented Sep 1, 2015

theforeman-bot commented Sep 1, 2015

dLobatog Sep 1, 2015

Choose a reason for hiding this comment

dLobatog commented Sep 1, 2015

dLobatog Sep 1, 2015

Choose a reason for hiding this comment

dLobatog commented Sep 1, 2015

theforeman-bot commented Sep 1, 2015

dLobatog commented Sep 1, 2015

dLobatog commented Sep 2, 2015

dLobatog Sep 2, 2015

Choose a reason for hiding this comment

theforeman-bot commented Sep 2, 2015

dLobatog commented Sep 3, 2015

theforeman-bot commented Sep 3, 2015

kairel commented Sep 4, 2015

domcleal commented Sep 4, 2015

kairel commented Sep 7, 2015

domcleal commented Sep 7, 2015

dLobatog Sep 10, 2015

Choose a reason for hiding this comment

dLobatog commented Sep 10, 2015

theforeman-bot commented Sep 11, 2015

dLobatog commented Sep 11, 2015

dLobatog commented Sep 11, 2015