Fixes #4127 - Global Parameters with types #5241

kgaikwad · 2018-02-07T10:09:10Z

No description provided.

theforeman-bot · 2018-02-07T10:09:18Z

Issues: #4127

houndci-bot · 2018-02-07T10:09:22Z

db/migrate/20171109095957_add_key_type_to_parameters.rb

+  end
+
+  def down
+    remove_column :parameters, :key_type 


Trailing whitespace detected.

houndci-bot · 2018-02-07T10:09:22Z

app/models/concerns/key_value_validation.rb

+    return if !self.value.is_a?(String) || value.contains_erb?
+    Foreman::Parameters::Caster.new(self, :attribute_name => :value, :to => object_for_key_type.key_type).cast!
+  rescue StandardError, SyntaxError => e
+      Foreman::Logging.exception("Error while parsing #{object_for_key_type}", e)


Use 2 (not 4) spaces for indentation.

houndci-bot · 2018-02-07T10:09:22Z

app/models/concerns/key_value_validation.rb

@@ -0,0 +1,12 @@
+module KeyValueValidation
+  extend ActiveSupport::Concern
+


Trailing whitespace detected.

houndci-bot · 2018-02-07T10:09:22Z

app/helpers/key_types_helper.rb

+                   "<dt>JSON</dt> <dd>Any valid JSON input.</dd>" +
+               "</dl>").html_safe,
+                 :label_help_options => { :title => _("How values are validated") }}
+


Trailing whitespace detected.

sean797

Thanks for doing this, it makes the input so much better!

I know its WIP but I guess there are API changes needed?

sean797 · 2018-02-07T19:30:21Z

db/migrate/20171109095957_add_key_type_to_parameters.rb

@@ -0,0 +1,9 @@
+class AddKeyTypeToParameters < ActiveRecord::Migration
+  def up
+    add_column :parameters, :key_type, :string,  :default =>  N_("string"), :limit => 255


I'm not sure N_("string") should be marked for translation?

sean797 · 2018-02-07T19:31:53Z

app/models/concerns/key_type.rb

@@ -0,0 +1,20 @@
+module KeyType
+  extend ActiveSupport::Concern
+  KEY_TYPES = [N_("string"), N_("boolean"), N_("integer"), N_("real"), N_("array"), N_("hash"), N_("yaml"), N_("json")]


It would be nice to avoid storing them here and in the lookup_key model.

It looks like they are in a comment in the lookup_key model already, it should probably be removed altogether to avoid confusion.

orrabin · 2018-02-25T12:46:47Z

app/models/concerns/key_type.rb

@@ -0,0 +1,20 @@
+module KeyType
+  extend ActiveSupport::Concern
+  KEY_TYPES = [N_("string"), N_("boolean"), N_("integer"), N_("real"), N_("array"), N_("hash"), N_("yaml"), N_("json")]


It looks like they are in a comment in the lookup_key model already, it should probably be removed altogether to avoid confusion.

orrabin · 2018-02-25T12:50:05Z

app/models/parameter.rb

@@ -56,6 +61,10 @@ def self.type_priority(type)
    PRIORITY.fetch(type.to_s.underscore.to_sym, nil) unless type.nil?
  end

+  def value_before_type_cast


Not sure you need this, in KeyValueValidation the value_before_type_cast can run with no parameters and then it will work on self.value which is what you are sending.

the naming here is confusing, what's the difference between value_before_type_casting and value_before_type_cast? it looks like this only returns early if error is present

Actually, previously we have used method value_before_type_cast as custom method in lookup_key but if I am correct 'ActiveModelalso provides*_before_type_cast(here * -> attribute_name) methods so modified it and declaredvalue_before_type_casting as our custom method used bylookup_keyto check casting by passing thevalue` .
Any other suggestion on method name for value_before_type_casting?

EDIT - modified method name from value_before_type_casting to format_value_before_type_cast

orrabin · 2018-02-25T12:56:44Z

app/views/common_parameters/_form.html.erb

-      <label class="control-label col-md-2" for="common_parameter_value"><%= _("Value") %></label>
-
-      <div class="col-md-9">
-        <div class="editor-container">


Maybe we can consult @amirfefer or @sharvit on how to allow the editor when in full screen mode?
Or @Rohoover could suggest another editor.

Background for this: we are using the ACE editor but when adding a type and making parameters look more like smart class parameters we need to show validations next to the editor and using the ACE would not look so good.
There was an attempt at some point to only show the ACE editor when entering full screen for easy editing but that failed.
Perhaps with the use of react this is now possible?
It will probably need to be in a separate PR.

@orrabin
Is there are screenshot or visual you can show me?

@Rohoover,

Please check below screenshots for your reference.

Current web-ui from Global Parameters page

web-ui after my change where I have removed existing editor and added key type dropdown. I found that for other parameters pages, we haven't used any editor except this page.

web-ui for Smart Class Parameters Page

orrabin · 2018-02-25T12:59:01Z

app/helpers/key_types_helper.rb

+  def param_type_selector(f, options = {})
+    common_extra_options = { :size => "col-md-4", :class=> "without_select2",
+                 :label_help => _("<dl>" +
+                   "<dt>String</dt> <dd>Everything is taken as a string.</dd>" +


I know you didn't write it and just moved it but maybe now is a good time to change the wording in this comment a little.

orrabin · 2018-02-25T13:13:20Z

db/migrate/20171109095957_add_key_type_to_parameters.rb

@@ -0,0 +1,9 @@
+class AddKeyTypeToParameters < ActiveRecord::Migration


I get this error when trying to run the migrations:

StandardError: Directly inheriting from ActiveRecord::Migration is not supported. Please specify the Rails release the migration was written for: class AddKeyTypeToParameters < ActiveRecord::Migration[4.2]

orrabin · 2018-02-25T13:41:27Z

The validations works nicely and the migration looks correct.

The hidden value checkbox is not working with the change, the js there probably searches for the editor and needs to be updated.

kgaikwad · 2018-02-26T09:00:56Z

@sean797 and @orrabin,

Thank you for reviewing the pull-request as well as for suggesting additional required changes.
I will do modifications as per your review comments and update the PR soon.

Rohoover

@orrabin

I think I understand. Would it be possible to put the same expand button you have on the Smart Class Parameters page on the Global Parameters page next to Value box. This would seem like a consistent handling.

I hope I understood the issue properly.

orrabin · 2018-03-04T08:44:39Z

@Rohoover yes I meant adding the expand button and checking if when expanding we could get the editor.
There were issues with trying to add ACE only when expanding.
My questions:

does it even make sense to show it only when expanding?
are there other editors we should consider here?

sbernhard · 2018-04-30T12:57:58Z

I would be really happy to see this PR in foreman as it would help us to configure foreman_ansible roles which require hash values like https://github.com/bennojoy/nginx

How to proceed to get it in?

kgaikwad · 2018-04-30T14:17:42Z

@sbernhard,
I am working on it.
Made changes as per review comments but still some part is remaining i.e UI integration under parameters Tab of other resources pages like hostgroup -> parameters Tab.
I will try to update it sooner.

houndci-bot · 2018-05-03T08:08:18Z

app/models/concerns/key_value_validation.rb

+    return if !self.value.is_a?(String) || value.contains_erb?
+    Foreman::Parameters::Caster.new(self, :attribute_name => :value, :to => object_for_key_type.key_type).cast!
+  rescue StandardError, SyntaxError => e
+      Foreman::Logging.exception("Error while parsing #{object_for_key_type}", e)


Layout/IndentationWidth: Use 2 (not 4) spaces for indentation.

kgaikwad · 2018-05-03T08:37:39Z

@orrabin, @sean797 and @sbernhard,

Apologize for delay in delivery of these changes!
I have updated the PR with all suggested changes. Please have a look and let me know if there is any other place where I need to handle parameter types.

Fixed all review comments by @orrabin
Added API changes as @sean797 mentioned above
Added field type under Parameters Tab for other places like organizations > edit > Parameters
Yaml rendering on host page
Added sorting for type column on global parameters list
Added expand button on edit parameter page as @Rohoover suggested
Fixed hidden checkbox functionality on global parameter edit page as it was broken by new changes.
Fix test cases if any failure found due to new changes
Add few test cases for these changes

@Rohoover,
Here are some screenshots for few pages where I have added type changes.

Global Parameters Page
Global Parameter Edit Page:
Organizations > Parameters Tab and same way on other parameters Tab

Rohoover

@kgaikwad

Just a bit picky here but can we get the input field to match the height of the expand button for the edit page?

xprazak2 · 2018-05-25T11:46:12Z

@kgaikwad, what is the status here?

kgaikwad · 2019-02-11T10:04:37Z

@ares, thank you for review. I have addressed almost all comments except migration change.
Here is the hammer PR link - theforeman/hammer-cli-foreman#409

EDIT - updated PR with the suggested modifications.

spoke with Ori, she's fine with dismissing, the review is out of date

ares

It's ready from code perspective, there are 2 tests failing now that needs to be fixed. It seems escaping has changed.

Also I found one little issue, the value in parameter form does seems to have smaller height than it should. It's not aligned with fullscreen button. Should be tiny css fix I hope. See screenshot

After this is fixed, I'm happy to click the green button. I'm looking forward to this a lot :-)

kgaikwad · 2019-02-21T17:21:29Z

@ares,
Updated PR which includes tests fixes and UI fix.

ares

Thanks @kgaikwad, I retested and confirmed it still works smoothly. I'm very happy to click the merge button, great work!

timogoebel · 2019-02-25T19:50:51Z

app/models/concerns/hostext/search.rb

@@ -65,7 +65,7 @@ module Search

      scoped_search :relation => :puppetclasses, :on => :name, :complete_value => true, :rename => :class, :only_explicit => true, :operators => ['= ', '~ '], :ext_method => :search_by_puppetclass
      scoped_search :relation => :fact_values, :on => :value, :in_key => :fact_names, :on_key => :name, :rename => :facts, :complete_value => true, :only_explicit => true, :ext_method => :search_cast_facts
-      scoped_search :relation => :search_parameters, :on => :value, :on_key => :name, :complete_value => true, :rename => :params, :ext_method => :search_by_params, :only_explicit => true


I know, I'm late to the party. But where did this go?

As value field is now converted to serialized one searching is not working as expected.
I tried using to_yaml but won't able to cover all scenarios and for smart_class_parameters, I don't find searching with value field.
One of suggestion given by @ekohl is to have searchable field separately and value with yaml.
After this discussion, I have removed searching parameters using value field and added searching based on name.
What is your thoughts on this?

I believe we (dm) use search by parameters in some automation via API requests. We (foreman) promise that the APIv2 is stable and I believe part of the promise is keeping the search stable.

I think the least we should do is add this to the manual in the upgrade notes.

can't we keep the current behavior ? this will continue to work with strings as it was prior to the change?

I think the search syntax can't be considered as part of the API. While we should try to do as little breaking changes as possible, but sometimes it does not make sense. From time to time we realize it's better to rename the search keyword for a given attribute or change the :only_explicit definition or drop some operators for some attribute and it was always accepted.

Now for the suggestion, if we allow fulltext searching, it could work in some cases. But it can also start returning weird results or simply feel buggy. People will ask for adding more operators in order to search in nested keys etc. I think it's better to just drop this and don't make people confused. I believe it's clear to all devs, how the search would behave, but it would be quite surprising for users who don't understand how we store data in SQL and how scoped search syntax translates to it.

Also given it doesn't work on smart class parameters and smart variables, I don't think it will hit majority of users. If you strongly believe we should add full text searching back, I'd prefer if we first add some inline help about how the search works and especially in this particular search field.

I think the search syntax can't be considered as part of the API.

I think the search syntax is a valid part of the API, we even use it for permissions. As I said, the least we should do is document the changes so users don't get surprises when they upgrade.

So here's the documentation - theforeman/theforeman.org#1322
Given the fact ale least two people feel differently about that, we should probably bring that up to discourse or document the outcome in our manual. At the end I don't mind steering any direction, but we should set the expectations for users. I didn't find any mention at https://theforeman.org/manuals/1.20/index.html#4.1.5Searching (or in API section linked from here)

theforeman-bot added Needs testing Not yet reviewed labels Feb 7, 2018

houndci-bot reviewed Feb 7, 2018

View reviewed changes

orrabin requested review from ares and orrabin February 7, 2018 10:11

sean797 suggested changes Feb 7, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Feb 7, 2018

orrabin previously requested changes Feb 25, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Feb 25, 2018

Rohoover reviewed Mar 1, 2018

View reviewed changes

kgaikwad force-pushed the 4127_global_params_with_types branch from 5bbd666 to f4ba49a Compare May 3, 2018 08:07

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

houndci-bot reviewed May 3, 2018

View reviewed changes

kgaikwad force-pushed the 4127_global_params_with_types branch from f4ba49a to be7e1c0 Compare May 3, 2018 08:11

kgaikwad changed the title ~~[WIP] Fixes #4127 - Global Parameters with types~~ Fixes #4127 - Global Parameters with types May 3, 2018

kgaikwad force-pushed the 4127_global_params_with_types branch from be7e1c0 to c48adb3 Compare May 3, 2018 08:18

Rohoover reviewed May 3, 2018

View reviewed changes

xprazak2 added Waiting on contributor and removed Needs re-review labels May 25, 2018

theforeman-bot added Waiting on contributor and removed Needs re-review labels Feb 3, 2019

kgaikwad force-pushed the 4127_global_params_with_types branch from f2cc385 to 88f8143 Compare February 11, 2019 11:24

theforeman-bot added Needs re-review and removed Waiting on contributor labels Feb 11, 2019

shiramax mentioned this pull request Feb 14, 2019

Fixes #20348 - Inconsistently escaped quotes for Host Parameter theforeman/hammer-cli#290

Closed

ares requested changes Feb 15, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Feb 15, 2019

kgaikwad force-pushed the 4127_global_params_with_types branch from 88f8143 to 45710dd Compare February 20, 2019 07:14

theforeman-bot added Needs re-review and removed Waiting on contributor labels Feb 20, 2019

kgaikwad force-pushed the 4127_global_params_with_types branch from 45710dd to 6922d3d Compare February 20, 2019 18:04

kgaikwad added 4 commits February 21, 2019 13:26

Fixes theforeman#4127 - Global Parameters with types

c70ebb3

Refs theforeman#4127 - refactored code related to parameter_type

bdf37bf

Refs theforeman#4127 - fixed method call to pass test in parameter_test

6b372e1

Refs theforeman#4127 - fixed smart_variable tests and textarea width

b89b8cb

kgaikwad force-pushed the 4127_global_params_with_types branch from 6922d3d to b89b8cb Compare February 21, 2019 13:27

ares approved these changes Feb 22, 2019

View reviewed changes

theforeman-bot removed the Needs re-review label Feb 22, 2019

ares approved these changes Feb 22, 2019

View reviewed changes

ares merged commit 62f2fd8 into theforeman:develop Feb 22, 2019

ares added Ready to merge Demo worthy and removed Needs testing labels Feb 22, 2019

timogoebel reviewed Feb 25, 2019

View reviewed changes

kgaikwad mentioned this pull request Feb 27, 2019

Add nightly headline feature - type support in Global Parameters theforeman/theforeman.org#1322

Merged

		@@ -0,0 +1,12 @@
		module KeyValueValidation
		extend ActiveSupport::Concern

		@@ -0,0 +1,9 @@
		class AddKeyTypeToParameters < ActiveRecord::Migration

Fixes #4127 - Global Parameters with types #5241

Fixes #4127 - Global Parameters with types #5241

Conversation

kgaikwad commented Feb 7, 2018

theforeman-bot commented Feb 7, 2018

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

sean797 left a comment • edited

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

kgaikwad Feb 11, 2019 • edited

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

orrabin commented Feb 25, 2018

kgaikwad commented Feb 26, 2018

Rohoover left a comment

Choose a reason for hiding this comment

orrabin commented Mar 4, 2018

sbernhard commented Apr 30, 2018

kgaikwad commented Apr 30, 2018

Choose a reason for hiding this comment

kgaikwad commented May 3, 2018

Rohoover left a comment

Choose a reason for hiding this comment

xprazak2 commented May 25, 2018 • edited

kgaikwad commented Feb 11, 2019 • edited

ares left a comment

Choose a reason for hiding this comment

kgaikwad commented Feb 21, 2019

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

Choose a reason for hiding this comment

sean797 left a comment •

edited

kgaikwad Feb 11, 2019 •

edited

xprazak2 commented May 25, 2018 •

edited

kgaikwad commented Feb 11, 2019 •

edited