Fixes #12566 - host_parameters_attributes accepts nested flag #2928

lzap · 2015-11-23T15:04:00Z

This is for compatibility reasons. I wish we had an expectation for deprecation
warnings, so we can actually silence them and also test them! Should not be too
difficult to do I guess, ideas?

domcleal · 2015-11-23T15:23:39Z

app/controllers/api/v2/hosts_controller.rb

@@ -82,6 +82,12 @@ def show
      param_group :host, :as => :create

      def create
+        params[:host][:host_parameters_attributes] && params[:host] && params[:host][:host_parameters_attributes].each do |a|


Testing params[:host] after accessing its :host_parameters_attributes member is the wrong order, as the hypothetical error would be on the first condition.

Oh, it's being tested twice - remove the first.

Nitpick, use a longer variable name than a please (attribute?).

On Mon, Nov 23, 2015 at 07:23:51AM -0800, Dominic Cleal wrote:

@@ -82,6 +82,12 @@ def show
param_group :host, :as => :create

def create

params[:host][:host_parameters_attributes] && params[:host] && params[:host][:host_parameters_attributes].each do |a|

Oh, it's being tested twice - remove the first.

Yeah, completely screwed copy+paste operation :-)

Later,
Lukas #lzap Zapletal

domcleal · 2015-11-23T15:28:15Z

I wish we had an expectation for deprecation
warnings, so we can actually silence them and also test them!

Expect it like any other method, i.e. Foreman::Deprecation.expects(:api_deprecation_warning).with(/foo/). No need to test the implementation in a functional test - if we need coverage of the inside, add it to test/unit/foreman_deprecation_test.rb.

Other comments:

missing change to the v1 controller
missing change to the update action on the v2 controller, use a filter like app/controllers/api/v2/operatingsystems_controller.rb (and others) to avoid duplication
apipie parameter docs can specify the format of the hash inside the parameter I think, so it should be possible to add a description to say the inner parameter's deprecated

lzap · 2015-11-24T13:34:33Z

missing change to the v1 controller

missing change to the update action on the v2 controller, use a filter like app/controllers/api/v2/operatingsystems_controller.rb (and others) to avoid duplication

apipie parameter docs can specify the format of the hash inside the parameter I think, so it should be possible to add a description to say the inner parameter's deprecated

All done, except the extra documentation desc field, I don't know
exactly what parameters are supported at the moment.

Later,
Lukas #lzap Zapletal

domcleal · 2015-11-24T13:42:03Z

I don't know exactly what parameters are supported at the moment.

Attributes of Parameter (name, value) and attributes that accepts_nested supports (_destroy)?

domcleal · 2015-11-24T13:42:50Z

app/controllers/api/v1/hosts_controller.rb

@@ -1,6 +1,9 @@
 module Api
  module V1
    class HostsController < V1::BaseController
+      include Api::CompatibilityChecker
+      before_filter :check_create_host_nested, :only => [:create]


Applicable for :update too?

Applicable for :update too?

Pushed, without unit test tho, don't think it's necessary.

Later,
Lukas #lzap Zapletal

domcleal · 2015-11-27T11:47:23Z

This only works for an array style set of nested attributes, it didn't work for me when I tried it the usual UI way (nested hashes), which confused me. The iteration over attributes doesn't take this into account:

2015-11-27T11:42:40 [app] [I] Started POST "/api/v2/hosts" for 127.0.0.1 at 2015-11-27 11:42:40 +0000
2015-11-27T11:42:41 [app] [I] Processing by Api::V2::HostsController#create as JSON
2015-11-27T11:42:41 [app] [I]   Parameters: {"host"=>{"name"=>"test12566a", "hostgroup_name"=>"libvirt", "compute_resource_name"=>"localhost", "host_parameters_attributes"=>{"0"=>{"name"=>"myparam", "value"=>"[FILTERED]", "nested"=>"true"}}}, "apiv"=>"v2"}
...
 | ActiveRecord::UnknownAttributeError: unknown attribute: nested

The tests aren't actually testing this change, only for parameters without the nested attribute.
API docs missing still?

lzap · 2015-11-30T14:15:54Z

API docs missing still?

How to document both array and hash input?

Later,
Lukas #lzap Zapletal

domcleal · 2015-11-30T14:17:02Z

I don't know, you'll have to look at apipie-bindings. Documenting one is probably fine.

lzap · 2015-11-30T14:22:04Z

I don't know, you'll have to look at apipie-bindings. Documenting one is probably fine.

Right, documented the array with a comment for hash.

Pushed.

Later,
Lukas #lzap Zapletal

ohadlevy · 2015-11-30T14:57:35Z

do we want to support both array and a hash? AFAIR one is for create the other for update?

domcleal · 2015-11-30T14:59:17Z

There are test failures, please take a look.

lzap · 2015-11-30T15:29:11Z

do we want to support both array and a hash? AFAIR one is for create the
other for update?

We do support both now, the checker just deletes the attribute and
issues a warning, so not a big deal to do this on both. Eventually we
will remove this.

Later,
Lukas #lzap Zapletal

lzap · 2015-11-30T15:34:57Z

There are test failures, please take a look.

Strange, my rubocop was/is green. Fixed.

Later,
Lukas #lzap Zapletal

domcleal · 2015-11-30T15:36:27Z

Strange, my rubocop was/is green. Fixed.

You may have to take into account that our Jenkins jobs are configured to test the PR when rebased/merged onto develop (to pick up any potential logical/non-patch conflicts). It's possible that a test passes on the branch but not when added to the current state of the base branch.

lzap · 2015-11-30T15:39:51Z

Damn squashed it into incorrect PR, hold on.

lzap · 2015-11-30T15:41:32Z

Pushed.

domcleal · 2015-12-01T11:42:30Z

app/controllers/api/v2/hosts_controller.rb

@@ -62,7 +65,10 @@ def show
          param :owner_type, Host::Base::OWNER_TYPES, :desc => N_("Host's owner type")
          param :puppet_ca_proxy_id, :number
          param :image_id, :number
-          param :host_parameters_attributes, Array
+          param :host_parameters_attributes, Array, :desc => N_("Host's parameters (array or indexed hash)") do
+            param :name, String, :desc => "Name of the parameter", :required => true


missing string extraction here

domcleal · 2015-12-03T09:01:09Z

app/controllers/api/v1/hosts_controller.rb

@@ -45,7 +48,10 @@ def show
        param :owner_id, :number
        param :puppet_ca_proxy_id, :number
        param :image_id, :number
-        param :host_parameters_attributes, Array
+          param :host_parameters_attributes, Array, :desc => N_("Host's parameters (array or indexed hash)") do


indentation's off here, and there's no need for translations in the v1 controller

domcleal · 2016-01-04T10:43:44Z

This missed 1.10.0 as the latest comment wasn't fixed, but remains on the .1 list. If you could rebase it and fix the comment, it'd be appreciated.

domcleal · 2016-01-12T13:01:43Z

Hello? Quite a trival comment, but please fix it and rebase the PR.

domcleal · 2016-01-14T12:50:03Z

Merged as and fixed up in 7ee381e.

theforeman-bot added Not yet reviewed Needs testing labels Nov 23, 2015

domcleal added the 1.10.0 label Nov 23, 2015

domcleal reviewed Nov 23, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Not yet reviewed labels Nov 23, 2015

lzap force-pushed the host-attrs-12566 branch from 3dda76d to 064b095 Compare November 24, 2015 13:31

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

domcleal reviewed Nov 24, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Nov 24, 2015

lzap force-pushed the host-attrs-12566 branch from 064b095 to 1a42b92 Compare November 26, 2015 15:10

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

domcleal added Waiting on contributor and removed Needs re-review labels Nov 27, 2015

lzap force-pushed the host-attrs-12566 branch from 1a42b92 to 27bc465 Compare November 30, 2015 14:22

theforeman-bot added Needs re-review and removed Waiting on contributor labels Nov 30, 2015

theforeman-bot added the Needs testing label Nov 30, 2015

domcleal added Waiting on contributor and removed Needs re-review labels Nov 30, 2015

lzap force-pushed the host-attrs-12566 branch from 27bc465 to 2fd6b76 Compare November 30, 2015 15:41

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

domcleal reviewed Dec 1, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Dec 1, 2015

Fixes #12566 - host_parameters_attributes accepts nested flag

43d09dd

lzap force-pushed the host-attrs-12566 branch from 2fd6b76 to 43d09dd Compare December 3, 2015 08:55

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

domcleal reviewed Dec 3, 2015
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Dec 3, 2015

domcleal closed this Jan 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #12566 - host_parameters_attributes accepts nested flag #2928

Fixes #12566 - host_parameters_attributes accepts nested flag #2928

lzap commented Nov 23, 2015

domcleal Nov 23, 2015

domcleal Nov 23, 2015

domcleal Nov 23, 2015

lzap Nov 24, 2015

domcleal commented Nov 23, 2015

lzap commented Nov 24, 2015

domcleal commented Nov 24, 2015

domcleal Nov 24, 2015

lzap Nov 26, 2015

domcleal commented Nov 27, 2015

lzap commented Nov 30, 2015

domcleal commented Nov 30, 2015

lzap commented Nov 30, 2015

ohadlevy commented Nov 30, 2015 via email

domcleal commented Nov 30, 2015

lzap commented Nov 30, 2015

lzap commented Nov 30, 2015

domcleal commented Nov 30, 2015

lzap commented Nov 30, 2015

lzap commented Nov 30, 2015

domcleal Dec 1, 2015

domcleal Dec 3, 2015

domcleal commented Jan 4, 2016

domcleal commented Jan 12, 2016

domcleal commented Jan 14, 2016

Fixes #12566 - host_parameters_attributes accepts nested flag #2928

Fixes #12566 - host_parameters_attributes accepts nested flag #2928

Conversation

lzap commented Nov 23, 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

domcleal commented Nov 23, 2015

lzap commented Nov 24, 2015

domcleal commented Nov 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented Nov 27, 2015

lzap commented Nov 30, 2015

domcleal commented Nov 30, 2015

lzap commented Nov 30, 2015

ohadlevy commented Nov 30, 2015 via email

domcleal commented Nov 30, 2015

lzap commented Nov 30, 2015

lzap commented Nov 30, 2015

domcleal commented Nov 30, 2015

lzap commented Nov 30, 2015

lzap commented Nov 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented Jan 4, 2016

domcleal commented Jan 12, 2016

domcleal commented Jan 14, 2016