fixes #9031 - Add routes to view template_combinations per hostgroup / env #2259

shlomizadok · 2015-03-24T09:18:56Z

Created under /api/v2/hostgroups/:id/template_combinations and /api/v2/environments/:id/template_combinations, as it seems more reasonable.

I have also added the option to update template combinations.

domcleal · 2015-05-01T10:18:47Z

app/controllers/api/v2/template_combinations_controller.rb

+        Environment.authorized(:view_templates).find(params[:environment_id]) rescue nil
+      end
+
+      def requested_resource


Is it possible to use the find_*_nested_object support in the API base controller here? It looks like you'd just specify allowed_nested_id and it could look up the base object.

domcleal · 2015-05-12T14:07:05Z

app/controllers/api/v2/template_combinations_controller.rb

@@ -28,6 +41,16 @@ def create
      def show
      end

+      api :PUT, "/config_templates/:config_template_id/template_combinations/:id", N_("Update template combination")
+      api :PUT, "/hostgroups/:hostgroup_id/template_combinations/:id", N_("Update template combination")
+      api :PUT, "/environments/:environment_id/template_combinations/:id", N_("Update template combination")


you need to define :config_template_id/hostgroup_id etc. They're already on the actions above, so I'd suggest creating a param_group and using that on all three actions to avoid copy/pasting it a third time.

Done, though I really wanted to copy and paste three times

domcleal · 2015-05-13T10:24:04Z

app/controllers/api/v2/template_combinations_controller.rb

      end

+      api :POST, "/config_templates/:config_template_id/template_combinations", N_("Add a template combination")
+      api :POST, "/hostgroups/:hostgroup_id/template_combinations", N_("Add a template combination")
+      api :POST, "/environments/:environment_id/template_combinations", N_("Add a template combination")


These routes don't seem to work, they're not listed in rake routes for me and I get: ActionController::RoutingError (No route matches [POST] "/api/v2/environments/production/template_combinations")

Seems to work now.

shlomizadok · 2015-05-13T12:17:20Z

Weird, my rake routes shows route to /api/v2/environments/production/template_combinations (http://fpaste.org/221463/31517887/ line 8), I am also able to post (yet getting double_render error.... [fixing])

domcleal · 2015-05-18T08:05:51Z

app/controllers/api/v2/template_combinations_controller.rb

+        @config_template = ConfigTemplate.authorized(:view_templates).find(params[:config_template_id]) rescue nil
+      end
+
+      def requested_resource


Is this necessary any more? It looks like find_parent_config_template is handled by the nested resource filter, and that the "return not_found" part could be handled by using find_required_nested_obj rather than optional. Or is there a subtle difference?

It is necessary, the subtle difference is that we'd like to return one of config_template OR nested_obj as the resource for the template combination (tests fail w/o assigning this resource).

Sorry, I don't think I was clear by putting this comment on the wrong line. I meant to ask whether find_parent_config_template is required, because config_template_id is in the allowed_nested_id list. Then everything is handled via the nested object lookup in the base controller, and all we need nested_obj.

e.g. domcleal@a298ae5

Tests pass for me, but to be honest, they're not very thorough...

domcleal · 2015-05-29T14:30:24Z

app/controllers/api/v2/template_combinations_controller.rb

@@ -36,9 +62,13 @@ def destroy
      end

      def find_parent_config_template


also unused? I can't see any references to @config_template, I'm pretty sure it's all going through nested_obj now.

Actually @config_template is used and is required by :index, :create (failing without it.)
I tried to removed it, as I thought it will be through nested_obj but those actions require it.

reference: https://github.com/shlomizadok/foreman/blob/fix_9031/app/controllers/api/v2/template_combinations_controller.rb#L6

I see in your link that this filter's called, but both methods called nested_obj. It seems to work here when I comment out the filter, what error do you see?

The error I see is me forgetting to remove the before_filter 😳

…hostgroup / environment

domcleal · 2015-06-01T07:55:21Z

Merged as ee66b3a, thanks @shlomizadok.

theforeman-bot added Not yet reviewed Needs testing labels Mar 24, 2015

shlomizadok closed this Apr 27, 2015

shlomizadok deleted the fix_9031 branch April 27, 2015 09:03

shlomizadok restored the fix_9031 branch April 27, 2015 09:27

shlomizadok reopened this Apr 27, 2015

domcleal reviewed May 1, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Not yet reviewed labels May 1, 2015

shlomizadok force-pushed the fix_9031 branch from 41b9bca to 06a0507 Compare May 12, 2015 14:00

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

domcleal reviewed May 12, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels May 12, 2015

shlomizadok force-pushed the fix_9031 branch from 06a0507 to 52a60a2 Compare May 12, 2015 14:28

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

domcleal reviewed May 13, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels May 13, 2015

shlomizadok force-pushed the fix_9031 branch from 52a60a2 to 349fc9e Compare May 13, 2015 12:23

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

domcleal reviewed May 18, 2015
View reviewed changes

domcleal added Waiting on contributor Needs re-review Needs testing and removed Needs re-review Needs testing Waiting on contributor labels May 18, 2015

shlomizadok force-pushed the fix_9031 branch from 349fc9e to ec2347d Compare May 28, 2015 11:29

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

domcleal reviewed May 29, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels May 29, 2015

fixes theforeman#9031 - Add routes to view template_combinations per …

82e0504

…hostgroup / environment

shlomizadok force-pushed the fix_9031 branch from ec2347d to 82e0504 Compare May 29, 2015 15:22

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

domcleal added Ready to merge and removed Needs re-review Needs testing labels Jun 1, 2015

domcleal closed this Jun 1, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #9031 - Add routes to view template_combinations per hostgroup / env #2259

fixes #9031 - Add routes to view template_combinations per hostgroup / env #2259

shlomizadok commented Mar 24, 2015

domcleal May 1, 2015

shlomizadok May 12, 2015

domcleal May 12, 2015

shlomizadok May 12, 2015

domcleal May 13, 2015

domcleal May 18, 2015

shlomizadok commented May 13, 2015

domcleal May 18, 2015

shlomizadok May 18, 2015

domcleal May 22, 2015

domcleal May 29, 2015

shlomizadok May 29, 2015

domcleal May 29, 2015

shlomizadok May 29, 2015

domcleal commented Jun 1, 2015

		@@ -36,9 +62,13 @@ def destroy
		end

		def find_parent_config_template

fixes #9031 - Add routes to view template_combinations per hostgroup / env #2259

fixes #9031 - Add routes to view template_combinations per hostgroup / env #2259

Conversation

shlomizadok commented Mar 24, 2015

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

Choose a reason for hiding this comment

shlomizadok commented May 13, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented Jun 1, 2015