-
Notifications
You must be signed in to change notification settings - Fork 985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixes #2810 - more restful API v2 JSON responses in puppetclasses #829
Conversation
@@ -0,0 +1,25 @@ | |||
module Api | |||
module V2 | |||
class HomeController < V2::BaseController |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason, a test failed that V2::HomeController wasn't defined, so I added it.
@domcleal, re-committed with tests and ready for review for merge. |
rebased |
[test] |
@@ -194,21 +194,32 @@ def find_nested_object | |||
if allowed_nested_id.include?(param) | |||
resource_identifying_attributes.each do |key| | |||
find_method = "find_by_#{key}" | |||
@nested_obj ||= $1.camelize.constantize.send(find_method, params[param]) | |||
klass = get_klass_name($1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm.. whats $1 in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ohadlevy, I updated the code to use the result of .match() rather than the global variable $1 for the first regex match.
re-based and re-squashed. previously failed on pg and Ruby 2 only. |
add_puppetmaster_filters :facts | ||
|
||
api :GET, "/hosts/:id/puppetrun", "Force a puppet run on the agent." | ||
|
||
def index | ||
@hosts = Host.my_hosts.search_for(*search_options).paginate(paginate_options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add this to the v2 controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@domcleal, I guess I was mixing PR's where I was adding v2 controllers and views to other objects. I will remove it in this PR.
Sorry I haven't been able to finish my review, got to head out. I'll try and get back to this soon, but I think it's post-1.3 code as it stands. |
@domcleal, rebased and cleaned up a bunch and fixed per your comments. I removed the API v2 changes to hosts, hostgroups, environments, etc, that don't relate specifically to puppletclasses, lookup_keys and lookup_values. |
@ohadlevy, I sent a separate commit to modify .classify() which is simpler than greating a separate method in Api::BaseController and then alias_method_chain in lookup_keys_common mix to override it. |
before_filter :find_required_nested_object, :only => [:index, :create] | ||
|
||
api :GET, '/smart_variables/:smart_variable_id/override_values', 'List of override values for a specific smart_variable' | ||
api :GET, '/smart_class_parameter/:smart_class_parameter_id/override_values', 'List of override values for a specific smart class parameter' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be "smart_class_parameters" (plural) and below.
I'm getting issues accessing override_values for a smart class parameter, e.g. Started GET "/api/puppetclasses/activemq/smart_class_parameters/112/override_values" for 127.0.0.1 at 2013-09-11 09:27:14 +0100 Processing by Api::V2::OverrideValuesController#index as JSON Parameters: {"puppetclass_id"=>"activemq", "smart_class_parameter_id"=>"112", "override_value"=>{}} User Load (0.2ms) SELECT "users".* FROM "users" WHERE "users"."login" = 'admin' LIMIT 1 AuthSource Load (0.2ms) SELECT "auth_sources".* FROM "auth_sources" WHERE "auth_sources"."id" = 1 LIMIT 1 User Load (0.1ms) SELECT "users".* FROM "users" WHERE (login='admin') LIMIT 1 Authenticated user Admin User against INTERNAL authentication source User Load (0.2ms) SELECT "users".* FROM "users" WHERE "users"."login" = 'admin' ORDER BY firstname LIMIT 1 Setting current user thread-local variable to admin (0.1ms) begin transaction (0.3ms) UPDATE "users" SET "last_login_on" = '2013-09-11 08:27:14.488362', "updated_at" = '2013-09-11 08:27:14.489910' WHERE "users"."id" = 1 (29.4ms) commit transaction Role Load (0.2ms) SELECT "roles".* FROM "roles" WHERE "roles"."name" = 'Anonymous' LIMIT 1 Role Exists (0.1ms) SELECT 1 AS one FROM "roles" INNER JOIN "user_roles" ON "roles"."id" = "user_roles"."role_id" WHERE "user_roles"."user_id" = 1 AND "roles"."id" = 8 LIMIT 1 Setting current user thread-local variable to admin Setting current user thread-local variable to nil User Load (0.2ms) SELECT "users".* FROM "users" WHERE "users"."login" = 'admin' LIMIT 1 Authorized user admin(Admin User) Setting current user thread-local variable to admin uninitialized constant SmartClassParameter (NameError) /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activesupport-3.2.14/lib/active_support/inflector/methods.rb:230:in `block in constantize' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activesupport-3.2.14/lib/active_support/inflector/methods.rb:229:in `each' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activesupport-3.2.14/lib/active_support/inflector/methods.rb:229:in `constantize' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activesupport-3.2.14/lib/active_support/core_ext/string/inflections.rb:54:in `constantize' It also seems that when nested beneath a puppet class, we should be able to use the parameter name rather than the ID, e.g. Started GET "/api/puppetclasses/activemq/smart_class_parameters/ensure" for 127.0.0.1 at 2013-09-11 09:42:33 +0100 Processing by Api::V2::SmartClassParametersController#show as JSON Parameters: {"puppetclass_id"=>"activemq", "id"=>"ensure", "smart_class_parameter"=>{}} User Load (0.2ms) SELECT "users".* FROM "users" WHERE "users"."login" = 'admin' LIMIT 1 AuthSource Load (0.1ms) SELECT "auth_sources".* FROM "auth_sources" WHERE "auth_sources"."id" = 1 LIMIT 1 User Load (0.1ms) SELECT "users".* FROM "users" WHERE (login='admin') LIMIT 1 Authenticated user Admin User against INTERNAL authentication source User Load (0.1ms) SELECT "users".* FROM "users" WHERE "users"."login" = 'admin' ORDER BY firstname LIMIT 1 Setting current user thread-local variable to admin (0.0ms) begin transaction (0.3ms) UPDATE "users" SET "last_login_on" = '2013-09-11 08:42:33.997703', "updated_at" = '2013-09-11 08:42:33.998309' WHERE "users"."id" = 1 (36.3ms) commit transaction Role Load (0.1ms) SELECT "roles".* FROM "roles" WHERE "roles"."name" = 'Anonymous' LIMIT 1 Role Exists (0.1ms) SELECT 1 AS one FROM "roles" INNER JOIN "user_roles" ON "roles"."id" = "user_roles"."role_id" WHERE "user_roles"."user_id" = 1 AND "roles"."id" = 8 LIMIT 1 Setting current user thread-local variable to admin Setting current user thread-local variable to nil User Load (0.2ms) SELECT "users".* FROM "users" WHERE "users"."login" = 'admin' LIMIT 1 Authorized user admin(Admin User) Setting current user thread-local variable to admin undefined method `find_by_name' for # (NoMethodError) /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activerecord-3.2.14/lib/active_record/dynamic_matchers.rb:27:in `method_missing' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activerecord-3.2.14/lib/active_record/relation/delegation.rb:14:in `block in find_by_name' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activerecord-3.2.14/lib/active_record/relation.rb:241:in `block in scoping' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activerecord-3.2.14/lib/active_record/scoping.rb:98:in `with_scope' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activerecord-3.2.14/lib/active_record/relation.rb:241:in `scoping' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/activerecord-3.2.14/lib/active_record/relation/delegation.rb:14:in `find_by_name' /home/dcleal/code/foreman/foreman/app/controllers/api/base_controller.rb:124:in `block in find_resource' /home/dcleal/code/foreman/foreman/app/controllers/api/base_controller.rb:120:in `each' /home/dcleal/code/foreman/foreman/app/controllers/api/base_controller.rb:120:in `find' /home/dcleal/code/foreman/foreman/app/controllers/api/base_controller.rb:120:in `find_resource' |
@domcleal, I ran into issues with protected/private on def resource_name in lookup_keys_common_controller and override_values_controller, so I removed protected/private for now. |
|
||
# overwrite so find_resource works for find_by_key, since there is no find_by_name | ||
def resource_identifying_attributes | ||
%w(id name key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this allows smart_variables/var_name (rather than smart_variables/id only)
alias_attribute :override_values_count, :lookup_values_count | ||
|
||
# to prevent errors caused by find_resource | ||
def self.find_by_name(str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handles can't find_by_name errors for smart_variables or smart_class_parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange, find_resource seems to call respond_to?, maybe that's returning true on the resource scope? https://github.com/isratrade/foreman/blob/9390e4c59f543d0ca561404066756ef86140c6c0/app/controllers/api/base_controller.rb#L123
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to debug this line as well. I couldn't understand (and still don't) why respond_to? passed as true for LookupKey.find_by_name and then through a NoMethodError: undefined method `find_by_name' ????
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess because it's implemented using method_missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LookupKey.respond_to?('find_by_foo')
=> false
so I'm confused how it passed on find_by_name ???
There seems to be an issue when referencing smart class parameters by name via nesting, as it isn't taking the nested puppet class into account when searching for the parameter so it's finding a parameter on another class with the same name. Here's an example: GET http://0.0.0.0:3000/api/puppetclasses/activemq::config/smart_class_parameters returns:
but GET http://0.0.0.0:3000/api/puppetclasses/activemq::config/smart_class_parameters/server_config returns:
if I query that parameter, GET http://0.0.0.0:3000/api/smart_class_parameters/11 returns:
Second issue is there's a difference in how the JSON's structured under smart_class_parameters, see:
There's an extra hash level inside the environment child. |
re-committed so that LookupKey.find_by_key will find the right one even though they aren't unique. This is probably the reason why in the UI, the id's are used instead. |
before_filter :find_environment, :only => [:index, :show, :update, :destroy] | ||
before_filter :find_puppetclass, :only => [:index, :show, :update, :destroy] | ||
before_filter :find_smart_class_parameters, :only => [:index, :show, :update, :destroy] | ||
before_filter :find_smart_class_parameter, :only => [:show, :update, :destroy] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not using find_resource, using find_smart_class_parameter instead, same for find_smart_variable
Not sure if you saw my IRC message. The unit test failure is due to the new fixture in test/fixtures/environment_classes.yml which adds an association between the production environment and the 'three' puppet class. The host unit test is explicitly ensuring this doesn't already exist. |
@smart_class_parameter ||= if LookupKey.smart_class_parameters.where(:key => params[:id]).count == 1 | ||
LookupKey.smart_class_parameters.find_by_key(params[:id]) | ||
else | ||
puppet_cond = "environment_classes.puppetclass_id = #{@puppetclass.id}" if @puppetclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a hash syntax of "environment_classes.puppetclass_id" => @puppetclass.id
would be a more secure than string interpolation, same below.
I'm seeing the same issue now where the override values controller doesn't support smart class parameters specified by name when nested... do we need a similar fix as was added to the smart class param controller? It could get a little messy as override_values is used for both smart class params and smart vars. |
Sorry, forgot the log output: Started GET "/api/puppetclasses/activemq::config/smart_class_parameters/server_config/override_values" for 127.0.0.1 at 2013-09-12 09:18:55 +0100 Processing by Api::V2::OverrideValuesController#index as JSON Parameters: {"puppetclass_id"=>"activemq::config", "smart_class_parameter_id"=>"server_config", "override_value"=>{}} User Load (0.2ms) SELECT "users".* FROM "users" WHERE "users"."id" = ? LIMIT 1 [["id", 1]] Setting current user thread-local variable to admin LookupKey Load (0.3ms) SELECT "lookup_keys".* FROM "lookup_keys" WHERE "lookup_keys"."id" = 0 ORDER BY lookup_keys.key LIMIT 1 LookupKey Load (0.3ms) SELECT "lookup_keys".* FROM "lookup_keys" WHERE "lookup_keys"."key" = 'server_config' ORDER BY lookup_keys.key LIMIT 1 LookupValue Load (0.1ms) SELECT "lookup_values".* FROM "lookup_values" WHERE "lookup_values"."lookup_key_id" = 11 LIMIT 20 OFFSET 0 Rendered api/v2/override_values/index.json.rabl (1.2ms) Body: {"override_values":[]} Completed 200 OK in 12ms (Views: 4.7ms | ActiveRecord: 0.8ms) Notice the "id = 0" SQL lookup in the middle. |
@domcleal, re-factored a bunch so I don't use Api::BaseController methods at all (find_resource, find_nested_obj, etc). Nearly all logic is in the mixin now for all 3 controllers, smart_variables, smart_class_parameters, and override_values. |
before_filter :find_host, :if => :host_id? | ||
before_filter :find_hostgroup, :if => :hostgroup_id? | ||
|
||
before_filter :find_smart_class_parameters, :if => :smart_class_parameter_id? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this accurate? It seems we should only do the full/index lookup when in the smart class parameters index action, not when an ID is passed and the reverse below, we shouldn't attempt an ID lookup when the index action is called.
It's looking much more reliable after the refactor, thanks. One small issue I see is that it doesn't pick up when an object isn't found now: GET /api/environments/asjkf/puppetclasses will show every puppet class as we don't check if the environment_id is passed, but no environment was found. |
@domcleal, I will fix this. I wanted to re-visit all the nested controllers and ensure that first object in the URL is found. I think it is currently only implemented for smart_variables, smart_class_parameters, and override_values. However, since this is a API-wide issue, not just on puppetclasses, is this a blocker for this PR? |
@isratrade ok, we can tackle that later. I went to merge this earlier but it broke two integration tests (Jenkins PR testing is offline at the moment so you won't see it there): # Running tests: ........................................F........E................................................. Finished tests in 516.585365s, 0.1916 tests/s, 0.5865 assertions/s. 1) Failure: HostgroupTest#test_0003_edit page [/home/dcleal/code/foreman/foreman/test/integration/hostgroup_test.rb:21]: redirect path /hostgroups was expected but it was /hostgroups/636252244-common. <"/hostgroups"> expected but was <"/hostgroups/636252244-common">. 2) Error: LookupKeyTest#test_0002_edit page: Capybara::ElementNotFound: Unable to find field "lookup_key_key" /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/capybara-2.0.3/lib/capybara/result.rb:22:in `find!' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/capybara-2.0.3/lib/capybara/node/finders.rb:26:in `block in find' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/capybara-2.0.3/lib/capybara/node/base.rb:78:in `synchronize' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/capybara-2.0.3/lib/capybara/node/finders.rb:26:in `find' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/capybara-2.0.3/lib/capybara/node/actions.rb:50:in `fill_in' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/capybara-2.0.3/lib/capybara/session.rb:341:in `block (2 levels) in ' /home/dcleal/.rvm/gems/ruby-2.0.0-p247@foreman/gems/capybara-2.0.3/lib/capybara/dsl.rb:51:in `block (2 levels) in ' /home/dcleal/code/foreman/foreman/test/integration/lookup_key_test.rb:14:in `block in ' |
…lasses NOTE: breaking changes to API v2 api/lookup_keys does not exist, replaced by api/smart_variables and api/smart_class_parameters
@domcleal, I removed the commit that re-opens the String class to modify .classify and I don't need any inflection changes, so I'm not inheriting anyway from base_controller for smart_variables, smart_class_parameters, and override_values |
[test] now the PR tester's back. |
Thanks @isratrade, this is very nice. Merged as 21bf889 for Foreman 1.3.0-RC2. |
NOTE: breaking changes to API v2 api/lookup_keys does not exist, replaced by api/smart_variables and api/smart_class_parameters