fixes #4198 - API v2 - add child nodes to show responses. Ex. architecture should show operating systems node #1208

isratrade · 2014-01-30T09:29:46Z

No description provided.

isratrade · 2014-01-30T11:20:21Z

app/views/api/v2/hostgroups/main.json.rabl

@@ -2,5 +2,6 @@ object @hostgroup

 extends "api/v2/hostgroups/base"

-attributes :subnet_id, :subnet_name, :operatingsystem_id, :operatingsystem_name, :domain_id, :domain_name, :environment_id, :environment_name,
-           :compute_profile_id, :compute_profile_name, :ancestry, :parameters, :puppetclass_ids


:parameters, :puppetclass_ids were removed here and moved to child nodes in show.json.rabl

domcleal · 2014-02-10T15:02:00Z

app/views/api/v2/config_templates/show.json.rabl

@@ -11,3 +11,11 @@ end
 child :operatingsystems, :object_root => false do
  extends "api/v2/operatingsystems/base"
 end
+
+child :environments, :object_root => false do


I'd say either remove this, or add host groups here, since config templates are associated with env/host.

Because we have template combinations though, I'd prefer to remove environments.

@domcleal, removing environments

domcleal · 2014-02-10T15:15:30Z

Could you check the test failure too please?

domcleal · 2014-02-17T08:42:50Z

Do you want to address http://projects.theforeman.org/issues/4349 too, and add orgs/loc child nodes?

isratrade · 2014-02-17T08:48:39Z

@domcleal, I was looking at this issue now.

isratrade · 2014-02-17T09:45:10Z

@domcleal, fixed, rebased, and included commit for fixes #4349

isratrade · 2014-02-17T12:49:35Z

this is really strange that the 2 failing tests pass locally for me. My is that SETTINGS[:locations_enabled] or SETTINGS[:organizations_enabled] is not being read as true.

isratrade · 2014-02-17T12:50:46Z

test/test_helper.rb

@@ -20,6 +20,8 @@
  # need to restart spork for it take effect.

  ENV["RAILS_ENV"] = "test"
+  SETTINGS[:locations_enabled] = true
+  SETTINGS[:organizations_enabled] = true


re-committed after adding these two lines

domcleal · 2014-02-17T15:22:10Z

@isratrade getting some test failures?

isratrade · 2014-02-17T16:34:33Z

The test failures are strange since it passes locally. I thought adding SETTINGS[:locations_enabled] = true to test_helper.rb would work, but it doesn't. Any ideas what could be on my local setup that isn't on Jenkins??

isratrade · 2014-02-17T16:43:06Z

@domcleal, I just ran the tests again and no failures on my local dev

domcleal · 2014-02-17T16:44:23Z

Jenkins already sets locations/orgs_enabled in settings.yaml before running tests, so there shouldn't be anything different....

domcleal · 2014-02-18T10:32:17Z

Something Bad happened on the last test run, I'd suggest running git commit --amend and re-pushing to get Jenkins to re-run.

In any case, I'm able to reproduce four test failures locally: https://gist.github.com/domcleal/9068336

domcleal · 2014-02-18T10:35:45Z

Quick investigation into the first error shows this response being formed:

Rendered api/v1/errors/unprocessable_entity.json.rabl (1.7ms)
Body: {"config_template":{"id":1007981701,"errors":{"base":["centos5_3_pxelinux is used by #<OsDefaultTemplate:0x000    0000d605b28>","centos5_3_pxelinux is used by #<OsDefaultTemplate:0x0000000d7480d0>"]},"full_messages":["centos5_3_pxelinux is used by #<OsDefaultTemplate:0x0000000d605b28>","centos5_3_pxelinux is used by #<OsDefaultTemplate:0x0000000d7480d0>"]}}
Completed 422 Unprocessable Entity in 192.3ms (Views: 8.4ms | ActiveRecord: 3.2ms)

But then later in the request, the error handling throws an error too, causing the response code to change to 500!

  1 resource have no errors (RuntimeError)
  2 /home/dcleal/code/foreman/foreman/app/controllers/api/base_controller.rb:74:in `process_resource_error'
  3 /home/dcleal/code/foreman/foreman/app/controllers/api/base_controller.rb:93:in `process_response'
  4 /home/dcleal/code/foreman/foreman/app/controllers/api/v1/config_templates_controller.rb:72:in `destroy'

domcleal · 2014-02-18T11:38:20Z

Hmm, I get entirely different failures to Jenkins, strange.

isratrade · 2014-02-18T13:53:05Z

created new branch from develop. cherry-picked and --amend both commits. all tests pass locally. let's hope they pass on our good friend jenkins

isratrade · 2014-02-18T14:26:27Z

still same 2 failing tests, can any else download this patch and run the tests locally. It's hard to debug when it passes for me locally. @GregSutcliffe, @elobato

dLobatog · 2014-02-18T20:45:23Z

Tested it locally, maybe this will shed some light on this:

(rdb:1) Domain.first.locations
[#<Location id: 255093256, name: "Location 1", type: "Location", created_at: "2014-02-18 20:08:44", updated_at: "2014-02-18 20:08:44", ignore_types: []>]
(rdb:1) Domain.first.organizations
[#<Organization id: 447626438, name: "Organization 1", type: "Organization", created_at: "2014-02-18 20:08:44", updated_at: "2014-02-18 20:08:44", ignore_types: []>]
   88        debugger
=> 89        assert show_response.keys.include?(node), "'#{node}' child node should be in response but was not"
   90      end
   91    end
   92  
   93  end
(rdb:1) show_response
{"id"=>22495316, "name"=>"mydomain.net", "fullname"=>nil, "dns_id"=>113629430, "created_at"=>"2014-02-18T20:08:44Z", "updated_at"=>"2014-02-18T20:08:44Z", "parameters"=>[{"id"=>665394701, "name"=>"parameter", "value"=>"value1"}], "interfaces"=>[], "taxonomies"=>[{"id"=>447626438, "name"=>"Organization 1"}], "subnets"=>[]}

The "taxonomies" subnode is created by

partial("api/v2/taxonomies/children_nodes", :object => domain)

instead of "organizations" and "locations".

isratrade · 2014-02-19T07:41:41Z

@elobato, here is what I get when I inspect show_response

{"id"=>22495316, "name"=>"mydomain.net", "fullname"=>nil, "dns_id"=>113629430, "created_at"=>"2014-02-19T07:36:03Z", "updated_at"=>"2014-02-19T07:36:03Z", "parameters"=>[{"id"=>665394701, "name"=>"parameter", "value"=>"value1"}], "interfaces"=>[], "locations"=>[{"id"=>255093256, "name"=>"Location 1"}], "organizations"=>[{"id"=>447626438, "name"=>"Organization 1"}], "subnets"=>[]}

I have nodes organizations" and "locations" and no node "taxonomies" like you have. I wonder what the difference in our code base.
I'm using rabl-0.8.6. I'll update it and see what happens.

isratrade · 2014-02-19T07:49:43Z

Gemfile

@@ -15,7 +15,7 @@ gem 'scoped_search', '>= 2.5'
 gem 'net-ldap'
 gem 'uuidtools'
 gem "apipie-rails", "~> 0.0.23"
-gem 'rabl', '>= 0.7.5', '<= 0.9.0'
+gem 'rabl', '>= 0.7.5', '< 0.9.0'


@elobato, here's the culprit. It fails with rabl 0.9.0. @domcleal, the latest rabl version is 0.9.3. Let's prevent users from using 0.9.x

@isratrade hm, this could be a problem as I know @jlsherrill updated the rabl RPM to 0.9.0 recently to fix something else. Can we dig in a bit deeper and identify the issue on 0.9.0?

@domcleal, will do, but in that case, let's allow all 0.9.x. Any reason to limit to 0.9.0?

@isratrade good point, I don't know why that was. I don't mind shipping a newer version, should be easy to build.

…x. architecture should show operating systems node

isratrade · 2014-02-19T13:18:52Z

app/views/api/v2/taxonomies/children_nodes.json.rabl

@@ -0,0 +1,11 @@
+if SETTINGS[:locations_enabled]
+  child :locations => :locations do


This should do the trick.

+ child :locations => :locations

I tried tries, but it causes an error and apparently :object_root => false isn't needed like I thought.

child :locations => :locations, :object_root => false

Somebody else also reported this issue from 0.9 to 0.9 on STI models nesquena/rabl#505

…nodes to user, domain, subnet, etc show json templates

isratrade · 2014-02-19T15:03:05Z

@elobato, @domcleal, passing tests on jenkins now, finally :)

domcleal · 2014-02-19T15:08:16Z

@isratrade nice one. There are still a few unresolved comments above, particularly for interfaces and environments versus template combinations - could you double check these please?

isratrade · 2014-02-19T16:12:01Z

@domcleal, changes per your and @GregSutcliffe's comments.
I removed all :object_root => false, since it doesn't seem to matter. I remember that it did previously, so I hope this doesn't come back to bite me.
You (or I) can squash last commit after your review.

domcleal · 2014-02-21T16:00:27Z

app/views/api/v2/operatingsystems/show.json.rabl

@@ -2,22 +2,30 @@ object @operatingsystem

 extends "api/v2/operatingsystems/main"

-child :media, :object_root => false do
+node do |operatingsystem|
+   { :parameters => partial("api/v2/parameters/base", :object => operatingsystem.parameters) }


[fixed] What's going on here, shouldn't this be a child node?

domcleal · 2014-02-21T16:43:03Z

Merged as d8b61e3, 8d113bd for Foreman 1.5.0, thanks @isratrade.

isratrade reviewed Jan 30, 2014
View reviewed changes

domcleal added Needs testing and removed Needs re-review labels Feb 6, 2014

domcleal reviewed Feb 10, 2014
View reviewed changes

domcleal added Waiting on contributor and removed Needs testing labels Feb 10, 2014

domcleal mentioned this pull request Feb 17, 2014

New authorization system #1070

Closed

domcleal added Needs re-review and removed Waiting on contributor labels Feb 17, 2014

isratrade reviewed Feb 17, 2014
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Feb 17, 2014

isratrade reviewed Feb 19, 2014
View reviewed changes

fixes theforeman#4198 - API v2 - add child nodes to show responses. E…

062a18b

…x. architecture should show operating systems node

isratrade reviewed Feb 19, 2014
View reviewed changes

fixes theforeman#4349 - API v2 - add location and organization child …

c9ba496

…nodes to user, domain, subnet, etc show json templates

domcleal added Needs re-review and removed Waiting on contributor labels Feb 19, 2014

squash this commit

bb48829

isratrade mentioned this pull request Feb 20, 2014

Fixes #4391: proxy ids are being sorted before the assertion #1247

Closed

domcleal reviewed Feb 21, 2014
View reviewed changes

domcleal closed this Feb 21, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #4198 - API v2 - add child nodes to show responses. Ex. architecture should show operating systems node #1208

fixes #4198 - API v2 - add child nodes to show responses. Ex. architecture should show operating systems node #1208

isratrade commented Jan 30, 2014

isratrade Jan 30, 2014

domcleal Feb 10, 2014

isratrade Feb 19, 2014

domcleal commented Feb 10, 2014

domcleal commented Feb 17, 2014

isratrade commented Feb 17, 2014

isratrade commented Feb 17, 2014

isratrade commented Feb 17, 2014

isratrade Feb 17, 2014

domcleal commented Feb 17, 2014

isratrade commented Feb 17, 2014

isratrade commented Feb 17, 2014

domcleal commented Feb 17, 2014

domcleal commented Feb 18, 2014

domcleal commented Feb 18, 2014

domcleal commented Feb 18, 2014

isratrade commented Feb 18, 2014

isratrade commented Feb 18, 2014

dLobatog commented Feb 18, 2014

isratrade commented Feb 19, 2014

isratrade Feb 19, 2014

domcleal Feb 19, 2014

isratrade Feb 19, 2014

domcleal Feb 19, 2014

isratrade Feb 19, 2014

isratrade commented Feb 19, 2014

domcleal commented Feb 19, 2014

isratrade commented Feb 19, 2014

domcleal Feb 21, 2014

domcleal commented Feb 21, 2014

		@@ -0,0 +1,11 @@
		if SETTINGS[:locations_enabled]
		child :locations => :locations do

fixes #4198 - API v2 - add child nodes to show responses. Ex. architecture should show operating systems node #1208

fixes #4198 - API v2 - add child nodes to show responses. Ex. architecture should show operating systems node #1208

Conversation

isratrade commented Jan 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented Feb 10, 2014

domcleal commented Feb 17, 2014

isratrade commented Feb 17, 2014

isratrade commented Feb 17, 2014

isratrade commented Feb 17, 2014

Choose a reason for hiding this comment

domcleal commented Feb 17, 2014

isratrade commented Feb 17, 2014

isratrade commented Feb 17, 2014

domcleal commented Feb 17, 2014

domcleal commented Feb 18, 2014

domcleal commented Feb 18, 2014

domcleal commented Feb 18, 2014

isratrade commented Feb 18, 2014

isratrade commented Feb 18, 2014

dLobatog commented Feb 18, 2014

isratrade commented Feb 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isratrade commented Feb 19, 2014

domcleal commented Feb 19, 2014

isratrade commented Feb 19, 2014

Choose a reason for hiding this comment

domcleal commented Feb 21, 2014