-
Notifications
You must be signed in to change notification settings - Fork 987
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 #26583 - Use ActiveSupport::JSON as RABL json engine #6669
Conversation
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
Not against, just curious why. Could you please provide some context even if it's draft? :-) |
@ares gave some context in the description. wanted to see if this breaks the build in any way |
489ada7
to
623bd27
Compare
thanks, makes even more sense now |
623bd27
to
730ea92
Compare
@@ -21,4 +21,6 @@ end | |||
# compatibility | |||
attribute :omit => :use_puppet_default | |||
|
|||
attribute :param_class, :as => :puppetclass_name | |||
node :puppetclass_name do |lk| |
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 was kind of interesting. Test failure came back reporting puppetclass_name as
{"id"=>298486374, "name"=>"apache", "created_at"=>"2019-04-10T17:25:22.845Z", "updated_at"=>"2019-04-10T17:25:22.845Z"}
rather than apache
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 sure if this raises any concerns
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.
Tested dates across some pages in the app..Looks good..
@@ -75,7 +75,7 @@ def call(template) | |||
# config.cache_engine = Rabl::CacheEngine.new # Defaults to Rails cache | |||
# config.perform_caching = false | |||
# config.escape_all_output = false | |||
# config.json_engine = nil # Class with #dump class method (defaults JSON) | |||
config.json_engine = ActiveSupport::JSON # Class with #dump class method (defaults JSON) |
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'm a bit concerned about this. We may be changing API responses, causing user scripting to break. We know that the date formats will change, but there may be other changes as well as a result of this - the tests did catch one, and there may be others that don't have coverage.
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 think the test failure has revealed that we're relying on odd behavior of ::JSON encoding:
Here's the object generated by RABL just before encoding (irrespective of json_engine and without my change)
=> {:description=>nil,
:override=>true,
:parameter_type=>"string",
:hidden_value?=>false,
:omit=>nil,
:required=>false,
:validator_type=>nil,
:validator_rule=>nil,
:merge_overrides=>false,
:merge_default=>false,
:avoid_duplicates=>false,
:override_value_order=>"fqdn\norganization,location\nhostgroup",
:created_at=>Mon, 15 Apr 2019 00:07:47 UTC +00:00,
:updated_at=>Mon, 15 Apr 2019 00:07:47 UTC +00:00,
:use_puppet_default=>nil,
:puppetclass_name=>
#<Puppetclass:0x000000000dcbd600
id: 298486374,
name: "apache",
created_at: Mon, 15 Apr 2019 00:07:47 UTC +00:00,
updated_at: Mon, 15 Apr 2019 00:07:47 UTC +00:00>,
:parameter=>"custom_class_param",
:id=>1018350795,
:puppetclass_id=>298486374,
:override_values_count=>0,
:default_value=>"abcdef",
:environments=>[{:id=>334344675, :name=>"production"}],
:override_values=>[]}
Note that puppetclass_name is the whole object, not its name.
Here's the encoding output with ::JSON
"{\"description\":null,\"override\":true,\"parameter_type\":\"string\",\"hidden_value?\":false,\"omit\":null,\"required\":false,\"validator_type\":null,\"validator_rule\":null,\"merge_overrides\":false,\"merge_default\":false,\"avoid_duplicates\":false,\"override_value_order\":\"fqdn\\norganization,location\\nhostgroup\",\"created_at\":\"2019-04-15 00:07:47 UTC\",\"updated_at\":\"2019-04-15 00:07:47 UTC\",\"use_puppet_default\":null,\"puppetclass_name\":\"apache\",\"parameter\":\"custom_class_param\",\"id\":1018350795,\"puppetclass_id\":298486374,\"override_values_count\":0,\"default_value\":\"abcdef\",\"environments\":[{\"id\":334344675,\"name\":\"production\"}],\"override_values\":[]}"
and with ActiveSupport::JSON
"{\"description\":null,\"override\":true,\"parameter_type\":\"string\",\"hidden_value?\":false,\"omit\":null,\"required\":false,\"validator_type\":null,\"validator_rule\":null,\"merge_overrides\":false,\"merge_default\":false,\"avoid_duplicates\":false,\"override_value_order\":\"fqdn\\norganization,location\\nhostgroup\",\"created_at\":\"2019-04-15T00:07:47.426Z\",\"updated_at\":\"2019-04-15T00:07:47.426Z\",\"use_puppet_default\":null,\"puppetclass_name\":{\"id\":298486374,\"name\":\"apache\",\"created_at\":\"2019-04-15T00:07:47.467Z\",\"updated_at\":\"2019-04-15T00:07:47.467Z\"},\"parameter\":\"custom_class_param\",\"id\":1018350795,\"puppetclass_id\":298486374,\"override_values_count\":0,\"default_value\":\"abcdef\",\"environments\":[{\"id\":334344675,\"name\":\"production\"}],\"override_values\":[]}"
My opinion is that the failure revealed by the test is due to an improperly declared attribute in the RABL template. If there are other cases like this they should be fixed. However, I don't see any other cases where we're declaring an attribute like :as => <something>_name
so I'm feeling pretty good about it.
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 wonder if @jyejare or someone else from QE can run some automation against this branch to check for any other changes in the api? Even changing the date format could possibly cause issues for users, and other changes could be even more difficult to handle.
Is there some way of working around this in katello to convert the date format before using it? I'm a bit reluctant to change API responses if we can avoid it.
Closing in favor of #6683 |
Looking into a reported Katello issue: https://projects.theforeman.org/issues/26417
Throughout the app where we display dates from API responses 'Invalid Date' is shown seemingly only in Firefox. My understanding is that Firefox apparently doesn't like non-ISO8601 dates:
Firefox w/ Date generated by ::JSON
Firefox w/ Date generated by ActiveSupport::JSON