-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
0b73677
to
1b5a19b
Compare
1b5a19b
to
de5a9ac
Compare
Thanks, @ekohl, updated. |
format: "puppet", | ||
version: 1, | ||
host: @body["host"], | ||
reported_at: @body["reported_at"], | ||
status: 0, | ||
proxy: @body["proxy"], | ||
body: @body, | ||
body: @body.to_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.
It looks like there is a TODO in build_report_root
that hints at this. Is it something that should be done in that method?
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.
Yeah this was not implemented yet:
# TODO add body string serialization and metric with total time
# and for tests there must be an option to turn this off
My thinking was that JSON in JSON is very hard to read so I was thinking to have some flag so in tests we could generate it without JSON in string so fixtures are easy to read and compare. This would need to be HTTP parameter so it can be used in integration tests.
Could you implement this?
de5a9ac
to
999bdc3
Compare
@lzap, updated. I've made |
999bdc3
to
bf38306
Compare
bf38306
to
10f6985
Compare
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.
Great tests will look still nice while we optimize for maximum efficiency.
I've changed the way the body is represented since it fixes the problem with https://github.com/theforeman/foreman_host_reports/blob/ab78021ca47d1b5676fd9c964206df975dc8ba3d/app/controllers/api/v2/host_reports_controller.rb#L50-L53. The problem is that if the body is not a string then it tries to generate one and the generated one cannot be
JSON.parse
d again. Also, in this way Rails won't parse body into parameters and thus we don't need to change the default behaviour by potentially breaking stuff. Since this plugin is aimed to be used only inforeman_host_reports
plugin context only, I guess we can afford such change.Also, made rubocop happy and fixed few tests.
P.S. With this change the report is being saved properly and parsed for view. There is also no additional parsing before save on the Foreman plugin side since the body is a string. The main concern I can see is that the body seems to be double
JSON.dump
ed.