Fixes #22893 - Specify taxonomies on template import #5323

xprazak2 · 2018-03-14T09:21:47Z

If any taxonomy params are passed for template import, they are used instead of what is in metadata.

theforeman-bot · 2018-03-14T09:21:57Z

houndci-bot · 2018-03-14T12:46:35Z

test/models/template_test.rb

        template.verify
      end

      test 'locking can be overriden by force option' do
        template = Minitest::Mock.new
        template.expect(:ignore_locking, true)
        Template.expects(:import_without_save => template)
-        Template.import!('test', '', :force => true)
+        Template.import!(name: 'test', text: '', options: :force => true)


unexpected token tASSOC

jyejare · 2018-03-19T14:37:58Z

ACK from QE.

Tests Performed:

1. POST without overridden taxonomies with some Provisioning Templates kinds- provision, script, snippet, finish, PXELinux.
Assert: All the POST requests for all the kinds work as expected.

2. POST with overridden taxonomies with some Provisioning Templates kinds- provision, script, snippet, finish, PXELinux.
Assert: All the POST requests for all the kinds work as expected.

3. POST without overridden taxonomies with Partition Table kind.
Assert: The POST request works as expected.

4. POST with overridden taxonomies Partition Table kind.
Assert: The POST request works as expected.

Extras(Regression Testing):

5. PUT to update the taxonomies of ptables and provisioning templates is not compromised.
Assert: PUT request works as expected.

Also, Please check the checks are failing.

xprazak2 · 2018-03-26T11:44:46Z

I found out passing organizations or locations did not work as expected so I removed them, now only organization_ids and organization_names are acceptable. Same for locations.

ares

thanks @xprazak2, please see comments inline, I'll continue with testing afterwards

ares · 2018-03-27T15:52:51Z

app/models/template.rb

@@ -82,8 +82,12 @@ def import_without_save(text, options = {})
    self.snippet = !!@importing_metadata[:snippet]
    self.locked = options[:lock] unless options[:lock].nil?

-    import_locations(options)
-    import_organizations(options)
+    if options[:taxonomy_params].empty?


what if I want to only override organizations? could we split this based on incoming params?

ares · 2018-03-27T15:55:19Z

app/models/template.rb

@@ -114,7 +118,7 @@ def self.parse_metadata(text)
  #   :force - set to true if you want to bypass locked templates
  #   :associate - either 'new', 'always' or 'never', determines when the template should associate objects based on metadata
  #   :lock - lock imported templates (false by default)
-  def self.import!(name, text, options = {})
+  def self.import!(name:, text:, options: {})


out of curiosity, why this is needed? the method import_attrs_for could have returned array and params could be passed with splat operator *

while I'm not against the change, I'm just curious why we change the method prototype

I reverted this back, changed import_params_for to return an array.

The change was not strictly needed, the suggested solution with array and splat works. I could also do:

def import_params_for(resource_name) # ... { :name => name, :template => template, :options => options } end # then in controller def import import_params = import_attrs_for(:provisioning_template) @provisioning_template = ProvisioningTemplate.import!(import_params[:name], import_params[:template], import_params[:options]) process_response @provisioning_template end

My reasoning was that when I need multiple different values returned from a method, hash is better option than array, because I do not have to keep track of order and I do not use array as if it were a tuple. And when I have proper keys in hash, why not use named params for destructuring instead of doing it manually.

I see, thanks, I think it's good refactoring in general, but I'd prefer to keep the change minimal for now as we're getting closer to 1.18.

ares · 2018-03-27T15:57:12Z

app/models/template.rb

@@ -146,6 +150,10 @@ def import_oses(options)
    end
  end

+  def import_taxonomy_override(taxonomy_params)
+    taxonomy_params.map  { |key, value| self.public_send("#{key}=", value) }


this does not seem very readable, I'd prefer to be more explicit and go over all enabled taxonomies https://github.com/theforeman/foreman/blob/develop/app/models/taxonomy.rb#L91

houndci-bot · 2018-03-28T12:34:57Z

app/models/template.rb

@@ -114,7 +113,7 @@ def self.parse_metadata(text)
  #   :force - set to true if you want to bypass locked templates
  #   :associate - either 'new', 'always' or 'never', determines when the template should associate objects based on metadata
  #   :lock - lock imported templates (false by default)
-  def self.import!(name, text, options = {})
+  def self.import!(name, text, options ={})


Surrounding space missing in default value assignment.

xprazak2 · 2018-03-28T12:37:22Z

Updated, it is now possible to override just locations or organizations.

ares

few more commemts, but getting there :-)

ares · 2018-03-28T15:56:13Z

app/controllers/concerns/foreman/controller/parameters/ptable.rb

@@ -21,4 +21,12 @@ def ptable_params_filter
  def ptable_params
    self.class.ptable_params_filter.filter_params(params, parameter_filter_context)
  end
+
+  def organization_params
+    self.class.organization_params_filter(::ProvisioningTemplate).filter_params(params, parameter_filter_context)


Should this use Ptable?

ares · 2018-03-28T15:56:31Z

app/controllers/concerns/foreman/controller/parameters/ptable.rb

+  end
+
+  def location_params
+    self.class.location_params_filter(::ProvisioningTemplate).filter_params(params, parameter_filter_context)


Also Ptable I think

ares · 2018-03-28T16:00:48Z

app/models/template.rb

+  def process_taxonomies(options, taxonomy)
+    tax_options = options["#{taxonomy}_params".to_sym]
+    if tax_options.empty?
+      send("import_#{taxonomy}s", options)


How about #{taxonomy.pluralize} just in case we ever another taxonomy with irregular pluralization rules (and to be consistent with the rest of the code)

ares · 2018-03-28T16:05:45Z

app/models/template.rb

@@ -146,6 +145,32 @@ def import_oses(options)
    end
  end

+  def import_taxonomies(options)
+    process_organizations options
+    process_locations options


I think this adds unnecessary layer, could we jusř use process_organizations a locations directly?

xprazak2 · 2018-03-29T06:47:30Z

I made changes as suggested.

ares

found just one thing, it would be good to document new api parameters using param_group :taxonomies, ::Api::V2::BaseController but then, it's good to go

houndci-bot

Some files could not be reviewed due to errors:

Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `S...

Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Performance/HashEachMethods` cop has been removed since it no longer provides performance benefits in modern rubies.

xprazak2 · 2018-04-04T10:48:10Z

Updated with api doc for taxonomies.

ares

ok, let's just wait for Mr. Jenkins to confirm

ares · 2018-04-04T14:01:16Z

Thanks @xprazak2, merging!

theforeman-bot added Needs testing Not yet reviewed labels Mar 14, 2018

xprazak2 force-pushed the tax-override branch from 3e11593 to dd4ed4d Compare March 14, 2018 12:46

houndci-bot reviewed Mar 14, 2018

View reviewed changes

xprazak2 force-pushed the tax-override branch from dd4ed4d to 69a7b32 Compare March 14, 2018 12:50

xprazak2 force-pushed the tax-override branch 3 times, most recently from bd2680a to 43b8e7b Compare March 26, 2018 11:43

xprazak2 force-pushed the tax-override branch 3 times, most recently from 8e1c2e0 to c8d4255 Compare March 27, 2018 07:29

ares requested changes Mar 27, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Mar 27, 2018

xprazak2 force-pushed the tax-override branch from c8d4255 to 330bfcf Compare March 28, 2018 12:34

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

houndci-bot reviewed Mar 28, 2018

View reviewed changes

xprazak2 force-pushed the tax-override branch from 330bfcf to ae54bde Compare March 28, 2018 12:35

ares requested changes Mar 28, 2018

View reviewed changes

xprazak2 force-pushed the tax-override branch from ae54bde to 83cf584 Compare March 29, 2018 06:47

ares requested changes Apr 4, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Apr 4, 2018

Fixes #22893 - Specify taxonomies on template import

e66d93e

xprazak2 force-pushed the tax-override branch from 83cf584 to e66d93e Compare April 4, 2018 10:47

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Apr 4, 2018

houndci-bot reviewed Apr 4, 2018

View reviewed changes

ares approved these changes Apr 4, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Apr 4, 2018

ares added Ready to merge and removed Needs testing labels Apr 4, 2018

ares merged commit cddb566 into theforeman:develop Apr 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #22893 - Specify taxonomies on template import #5323

Fixes #22893 - Specify taxonomies on template import #5323

xprazak2 commented Mar 14, 2018

theforeman-bot commented Mar 14, 2018

houndci-bot Mar 14, 2018

jyejare commented Mar 19, 2018 •

edited

Loading

xprazak2 commented Mar 26, 2018

ares left a comment

ares Mar 27, 2018

ares Mar 27, 2018

xprazak2 Mar 28, 2018

ares Mar 28, 2018

ares Mar 27, 2018

houndci-bot Mar 28, 2018

xprazak2 commented Mar 28, 2018

ares left a comment

ares Mar 28, 2018

ares Mar 28, 2018

ares Mar 28, 2018

ares Mar 28, 2018

xprazak2 commented Mar 29, 2018

ares left a comment

houndci-bot left a comment

xprazak2 commented Apr 4, 2018

ares left a comment

ares commented Apr 4, 2018

Fixes #22893 - Specify taxonomies on template import #5323

Fixes #22893 - Specify taxonomies on template import #5323

Conversation

xprazak2 commented Mar 14, 2018

theforeman-bot commented Mar 14, 2018

Choose a reason for hiding this comment

jyejare commented Mar 19, 2018 • edited Loading

xprazak2 commented Mar 26, 2018

ares left a 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

Choose a reason for hiding this comment

xprazak2 commented Mar 28, 2018

ares left a 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

xprazak2 commented Mar 29, 2018

ares left a comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

xprazak2 commented Apr 4, 2018

ares left a comment

Choose a reason for hiding this comment

ares commented Apr 4, 2018

jyejare commented Mar 19, 2018 •

edited

Loading