Skip to content

Commit

Permalink
Fixes #35885 - Error printing report with multiple results (#583)
Browse files Browse the repository at this point in the history 
* Fixes #35885 - Error printing report with multiple results

If Ansible job reported more than one results per action
then the config report page will fail to render. For example,
a task uses the Ansible copy module to copy multiple files
in a loop.

* Refs #35885 - fix rubocop issue

RuboCop::Cop::Lint::ShadowingOuterLocalVariable

module_args was in the `ansible_module_message` method scop,
and also as a parameter in the `get_results` function's signature

decided to remove the first definition of it as it wasn't used.

* Refs #35885 -  `end` was ommited during rebase

* Refs #35885 - fix tests undefined method `[]' for nil:NilClass

in case that `result['invocation']` is nil:
foreman_ansible/app/helpers/foreman_ansible/ansible_reports_helper.rb:98:in `block in get_results'

Co-authored-by: Ron Lavi <1ronlavi@gmail.com>
  • Loading branch information
@hao-yu @Ron-Lavi @nofaralfasi
2 people authored and nofaralfasi committed Jan 26, 2023
1 parent e91e0d2 commit cf4ab45
Show file tree
Hide file tree
Showing 4 changed files with 157 additions and 13 deletions.
35 changes: 26 additions & 9 deletions app/helpers/foreman_ansible/ansible_reports_helper.rb
View file
Expand Up @@ -32,20 +32,29 @@ def ansible_module_message(log)
when 'package'
msg_json['results'].empty? ? msg_json['msg'] : msg_json['results']
when 'template'
module_args = msg_json['invocation']['module_args']
_("Rendered template #{module_args['_original_basename']} to #{msg_json['dest']}")
get_results(msg_json) do |module_args, result|
_("Rendered template #{module_args['_original_basename']} to #{result['dest']}")
end
when 'service'
_("Service #{msg_json['name']} #{msg_json['state']} (enabled: #{msg_json['enabled']})")
get_results(msg_json) do |_, result|
_("Service #{result['name']} #{result['state']} (enabled: #{result['enabled']})")
end
when 'group'
_("User group #{msg_json['name']} #{msg_json['state']}, gid: #{msg_json['gid']}")
get_results(msg_json) do |_, result|
_("User group #{result['name']} #{result['state']}, gid: #{result['gid']}")
end
when 'user'
_("User #{msg_json['name']} #{msg_json['state']}, uid: #{msg_json['uid']}")
get_results(msg_json) do |_, result|
_("User #{result['name']} #{result['state']}, uid: #{result['uid']}")
end
when 'cron'
module_args = msg_json['invocation']['module_args']
_("Cron job: #{module_args['minute']} #{module_args['hour']} #{module_args['day']} #{module_args['month']} #{module_args['weekday']} #{module_args['job']} (disabled: #{module_args['disabled']})")
get_results(msg_json) do |module_args, _|
_("Cron job: #{module_args['minute']} #{module_args['hour']} #{module_args['day']} #{module_args['month']} #{module_args['weekday']} #{module_args['job']} (disabled: #{module_args['disabled']})")
end
when 'copy'
module_args = msg_json['invocation']['module_args']
_("Copy #{module_args['_original_basename']} to #{msg_json['dest']}")
get_results(msg_json) do |module_args, result|
_("Copy #{module_args['_original_basename']} to #{result['dest']}")
end
when 'command', 'shell'
msg_json['stdout_lines']
else
Expand All @@ -69,6 +78,14 @@ def ansible_report?(log)

private

def get_results(msg_json)
results = msg_json.key?('results') ? msg_json['results'] : [msg_json]
results.map do |result|
module_args = result.fetch('invocation', {}).fetch('module_args', {})
yield module_args, result
end
end

def parsed_message_json(log)
JSON.parse(log.message.value)
rescue StandardError => e
Expand Down
108 changes: 106 additions & 2 deletions test/fixtures/report.json
View file
@@ -1,4 +1,4 @@
{
[{
"reporter": "ansible",
"reported_at":"2018-01-15 17:31:36 521275",
"metrics": {
Expand All @@ -24,4 +24,108 @@
}
}
]
}
},
{
"reporter": "ansible",
"reported_at":"2022-12-22 10:52:48 521275",
"metrics": {
"time":
{ "total":133 }
},
"host": "io.local",
"status": {
"applied": 8,
"failed": 0,
"skipped": 0
},
"logs": [
{
"log": {
"sources": {
"source": "Schedule multiple cronjobs"
},
"messages": {
"message": "{\"changed\": true, \"failed\": false, \"module\": \"cron\", \"msg\": \"All items completed\", \"results\": [{\"_ansible_item_label\": \"date\", \"_ansible_no_log\": false, \"ansible_loop_var\": \"item\", \"changed\": true, \"envs\": [], \"failed\": false, \"invocation\": {\"module_args\": {\"backup\": false, \"cron_file\": null, \"day\": \"*\", \"disabled\": false, \"env\": null, \"hour\": \"5,2\", \"insertafter\": null, \"insertbefore\": null, \"job\": \"date > /dev/null\", \"minute\": \"0\", \"month\": \"*\", \"name\": \"Cron date\", \"reboot\": false, \"special_time\": null, \"state\": \"present\", \"user\": null, \"weekday\": \"*\"}}, \"item\": \"date\", \"jobs\": [\"Cron date\"]}, {\"_ansible_item_label\": \"df\", \"_ansible_no_log\": false, \"ansible_loop_var\": \"item\", \"changed\": true, \"envs\": [], \"failed\": false, \"invocation\": {\"module_args\": {\"backup\": false, \"cron_file\": null, \"day\": \"*\", \"disabled\": false, \"env\": null, \"hour\": \"5,2\", \"insertafter\": null, \"insertbefore\": null, \"job\": \"df > /dev/null\", \"minute\": \"0\", \"month\": \"*\", \"name\": \"Cron df\", \"reboot\": false, \"special_time\": null, \"state\": \"present\", \"user\": null, \"weekday\": \"*\"}}, \"item\": \"df\", \"jobs\": [\"Cron date\", \"Cron df\"]}]}"
},
"level": "notice"
}
},
{
"log": {
"sources": {
"source": "Schedule one cronjob"
},
"messages": {
"message": "{\"_ansible_no_log\": false, \"changed\": true, \"envs\": [], \"failed\": false, \"invocation\": {\"module_args\": {\"backup\": false, \"cron_file\": null, \"day\": \"*\", \"disabled\": false, \"env\": null, \"hour\": \"5,2\", \"insertafter\": null, \"insertbefore\": null, \"job\": \"hostname > /dev/null\", \"minute\": \"0\", \"month\": \"*\", \"name\": \"Schedule hostname\", \"reboot\": false, \"special_time\": null, \"state\": \"present\", \"user\": null, \"weekday\": \"*\"}}, \"jobs\": [\"Cron date\", \"Cron df\", \"Schedule hostname\"], \"module\": \"cron\"}"
},
"level": "notice"
}
},
{
"log": {
"sources": {
"source": "Render multiple templates"
},
"messages": {
"message": "{\"changed\": true, \"failed\": false, \"module\": \"template\", \"msg\": \"All items completed\", \"results\": [{\"_ansible_item_label\": \"test1.txt\", \"_ansible_no_log\": false, \"ansible_loop_var\": \"item\", \"changed\": true, \"checksum\": \"dba7673010f19a94af4345453005933fd511bea9\", \"dest\": \"/tmp/test1.txt\", \"diff\": [], \"failed\": false, \"gid\": 0, \"group\": \"root\", \"invocation\": {\"module_args\": {\"_original_basename\": \"test1.txt.j2\", \"attributes\": null, \"backup\": false, \"checksum\": \"dba7673010f19a94af4345453005933fd511bea9\", \"content\": null, \"delimiter\": null, \"dest\": \"/tmp/test1.txt\", \"directory_mode\": null, \"follow\": false, \"force\": true, \"group\": null, \"local_follow\": null, \"mode\": null, \"owner\": null, \"regexp\": null, \"remote_src\": null, \"selevel\": null, \"serole\": null, \"setype\": null, \"seuser\": null, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670606.02-7241-175625077259447/source\", \"unsafe_writes\": false, \"validate\": null}}, \"item\": \"test1.txt\", \"md5sum\": null, \"mode\": \"0644\", \"owner\": \"root\", \"secontext\": \"unconfined_u:object_r:admin_home_t:s0\", \"size\": 6, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670606.02-7241-175625077259447/source\", \"state\": \"file\", \"uid\": 0}, {\"_ansible_item_label\": \"test2.txt\", \"_ansible_no_log\": false, \"ansible_loop_var\": \"item\", \"changed\": true, \"checksum\": \"9054fbe0b622c638224d50d20824d2ff6782e308\", \"dest\": \"/tmp/test2.txt\", \"diff\": [], \"failed\": false, \"gid\": 0, \"group\": \"root\", \"invocation\": {\"module_args\": {\"_original_basename\": \"test2.txt.j2\", \"attributes\": null, \"backup\": false, \"checksum\": \"9054fbe0b622c638224d50d20824d2ff6782e308\", \"content\": null, \"delimiter\": null, \"dest\": \"/tmp/test2.txt\", \"directory_mode\": null, \"follow\": false, \"force\": true, \"group\": null, \"local_follow\": null, \"mode\": null, \"owner\": null, \"regexp\": null, \"remote_src\": null, \"selevel\": null, \"serole\": null, \"setype\": null, \"seuser\": null, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670620.49-7241-225254470383476/source\", \"unsafe_writes\": false, \"validate\": null}}, \"item\": \"test2.txt\", \"md5sum\": null, \"mode\": \"0644\", \"owner\": \"root\", \"secontext\": \"unconfined_u:object_r:admin_home_t:s0\", \"size\": 6, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670620.49-7241-225254470383476/source\", \"state\": \"file\", \"uid\": 0}]}"
},
"level": "notice"
}
},
{
"log": {
"sources": {
"source": "Render one template"
},
"messages": {
"message": "{\"_ansible_no_log\": false, \"changed\": true, \"checksum\": \"41c5985fc771b6ecfe8feaa99f8fa9b77ac7d6ce\", \"dest\": \"/tmp/test3.txt\", \"diff\": [], \"failed\": false, \"gid\": 0, \"group\": \"root\", \"invocation\": {\"module_args\": {\"_original_basename\": \"test3.txt.j2\", \"attributes\": null, \"backup\": false, \"checksum\": \"41c5985fc771b6ecfe8feaa99f8fa9b77ac7d6ce\", \"content\": null, \"delimiter\": null, \"dest\": \"/tmp/test3.txt\", \"directory_mode\": null, \"follow\": false, \"force\": true, \"group\": null, \"local_follow\": null, \"mode\": null, \"owner\": null, \"regexp\": null, \"remote_src\": null, \"selevel\": null, \"serole\": null, \"setype\": null, \"seuser\": null, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670634.79-7306-243717749452063/source\", \"unsafe_writes\": false, \"validate\": null}}, \"md5sum\": null, \"mode\": \"0644\", \"module\": \"template\", \"owner\": \"root\", \"secontext\": \"unconfined_u:object_r:admin_home_t:s0\", \"size\": 6, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670634.79-7306-243717749452063/source\", \"state\": \"file\", \"uid\": 0}"
},
"level": "notice"
}
},
{
"log": {
"sources": {
"source": "Copy multiple local files"
},
"messages": {
"message": "{\"changed\": true, \"failed\": false, \"module\": \"copy\", \"msg\": \"All items completed\", \"results\": [{\"_ansible_item_label\": \"test4.txt\", \"_ansible_no_log\": false, \"ansible_loop_var\": \"item\", \"changed\": true, \"checksum\": \"ab2649b7e58f7e32b0c75be95d11e2979399d392\", \"dest\": \"/tmp/test4.txt\", \"diff\": [], \"failed\": false, \"gid\": 0, \"group\": \"root\", \"invocation\": {\"module_args\": {\"_original_basename\": \"test4.txt\", \"attributes\": null, \"backup\": false, \"checksum\": \"ab2649b7e58f7e32b0c75be95d11e2979399d392\", \"content\": null, \"delimiter\": null, \"dest\": \"/tmp/test4.txt\", \"directory_mode\": null, \"follow\": false, \"force\": true, \"group\": \"root\", \"local_follow\": null, \"mode\": 256, \"owner\": \"root\", \"regexp\": null, \"remote_src\": null, \"selevel\": null, \"serole\": null, \"setype\": null, \"seuser\": null, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670648.88-7343-91799014257533/source\", \"unsafe_writes\": false, \"validate\": null}}, \"item\": \"test4.txt\", \"md5sum\": null, \"mode\": \"0400\", \"owner\": \"root\", \"secontext\": \"unconfined_u:object_r:admin_home_t:s0\", \"size\": 6, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670648.88-7343-91799014257533/source\", \"state\": \"file\", \"uid\": 0}, {\"_ansible_item_label\": \"test5.txt\", \"_ansible_no_log\": false, \"ansible_loop_var\": \"item\", \"changed\": true, \"checksum\": \"4ea77484f3a1c7dde4c0cca2f5c40953388f19f5\", \"dest\": \"/tmp/test5.txt\", \"diff\": [], \"failed\": false, \"gid\": 0, \"group\": \"root\", \"invocation\": {\"module_args\": {\"_original_basename\": \"test5.txt\", \"attributes\": null, \"backup\": false, \"checksum\": \"4ea77484f3a1c7dde4c0cca2f5c40953388f19f5\", \"content\": null, \"delimiter\": null, \"dest\": \"/tmp/test5.txt\", \"directory_mode\": null, \"follow\": false, \"force\": true, \"group\": \"root\", \"local_follow\": null, \"mode\": 256, \"owner\": \"root\", \"regexp\": null, \"remote_src\": null, \"selevel\": null, \"serole\": null, \"setype\": null, \"seuser\": null, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670662.97-7343-50902792283881/source\", \"unsafe_writes\": false, \"validate\": null}}, \"item\": \"test5.txt\", \"md5sum\": null, \"mode\": \"0400\", \"owner\": \"root\", \"secontext\": \"unconfined_u:object_r:admin_home_t:s0\", \"size\": 6, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670662.97-7343-50902792283881/source\", \"state\": \"file\", \"uid\": 0}]}"
},
"level": "notice"
}
},
{
"log": {
"sources": {
"source": "Copy one local files"
},
"messages": {
"message": "{\"_ansible_no_log\": false, \"changed\": true, \"checksum\": \"ec4cddb45c3ce640bed61b3d8ab6c18e715dac78\", \"dest\": \"/tmp/test6.txt\", \"diff\": [], \"failed\": false, \"gid\": 0, \"group\": \"root\", \"invocation\": {\"module_args\": {\"_original_basename\": \"test6.txt\", \"attributes\": null, \"backup\": false, \"checksum\": \"ec4cddb45c3ce640bed61b3d8ab6c18e715dac78\", \"content\": null, \"delimiter\": null, \"dest\": \"/tmp/test6.txt\", \"directory_mode\": null, \"follow\": false, \"force\": true, \"group\": \"root\", \"local_follow\": null, \"mode\": 256, \"owner\": \"root\", \"regexp\": null, \"remote_src\": null, \"selevel\": null, \"serole\": null, \"setype\": null, \"seuser\": null, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670677.05-7408-75605497546833/source\", \"unsafe_writes\": false, \"validate\": null}}, \"md5sum\": null, \"mode\": \"0400\", \"module\": \"copy\", \"owner\": \"root\", \"secontext\": \"unconfined_u:object_r:admin_home_t:s0\", \"size\": 6, \"src\": \"/root/.ansible/tmp/ansible-tmp-1671670677.05-7408-75605497546833/source\", \"state\": \"file\", \"uid\": 0}"
},
"level": "notice"
}
},
{
"log": {
"sources": {
"source": "Restart multiple services"
},
"messages": {
"message": "{\"changed\":true,\"failed\":false,\"module\":\"service\",\"msg\":\"All items completed\",\"results\":[{\"_ansible_item_label\":\"chronyd\",\"_ansible_no_log\":false,\"ansible_loop_var\":\"item\",\"changed\":true,\"failed\":false,\"invocation\":{\"module_args\":{\"daemon_reexec\":false,\"daemon_reload\":false,\"enabled\":null,\"force\":null,\"masked\":null,\"name\":\"chronyd\",\"no_block\":false,\"scope\":null,\"state\":\"restarted\",\"user\":null}},\"item\":\"chronyd\",\"name\":\"chronyd\",\"state\":\"started\"},{\"_ansible_item_label\":\"firewalld\",\"_ansible_no_log\":false,\"ansible_loop_var\":\"item\",\"changed\":true,\"failed\":false,\"invocation\":{\"module_args\":{\"daemon_reexec\":false,\"daemon_reload\":false,\"enabled\":null,\"force\":null,\"masked\":null,\"name\":\"firewalld\",\"no_block\":false,\"scope\":null,\"state\":\"restarted\",\"user\":null}},\"item\":\"firewalld\",\"name\":\"firewalld\",\"state\":\"started\"}]}"
},
"level": "notice"
}
},
{
"log": {
"sources": {
"source": "Restart one service"
},
"messages": {
"message": "{\"_ansible_no_log\":false,\"changed\":true,\"failed\":false,\"invocation\":{\"module_args\":{\"daemon_reexec\":false,\"daemon_reload\":false,\"enabled\":null,\"force\":null,\"masked\":null,\"name\":\"chronyd\",\"no_block\":false,\"scope\":null,\"state\":\"restarted\",\"user\":null}},\"module\":\"service\",\"name\":\"chronyd\",\"state\":\"started\"}"
},
"level": "notice"
}
}
]
}]
6 changes: 4 additions & 2 deletions test/unit/concerns/config_reports_extensions_test.rb
View file
Expand Up @@ -4,13 +4,15 @@

# Tests for the behavior of Host with roles, checks inheritance, etc
class ConfigReportExtensionsTest < ActiveSupport::TestCase
let(:example_report) do
let(:example_reports) do
JSON.parse(File.read(ansible_fixture_file('report.json')))
end

let(:example_report1) { example_reports.first }

describe '.import' do
it 'sets an origin for Ansible reports' do
report = ConfigReport.import(example_report)
report = ConfigReport.import(example_report1)
assert_equal 'Ansible', report.origin
end

Expand Down
21 changes: 21 additions & 0 deletions test/unit/helpers/ansible_reports_helper_test.rb
View file
Expand Up @@ -19,4 +19,25 @@ class AnsibleReportsHelperTest < ActiveSupport::TestCase
ansible_module_message(log).to_s
)
end

test 'module message extraction with action' do
example_report = JSON.parse(File.read(ansible_fixture_file('report.json'))).second
report = ConfigReport.import(example_report)
expected_outputs = [
'No additional data',
['Cron job: 0 5,2 * * * date > /dev/null (disabled: false)', 'Cron job: 0 5,2 * * * df > /dev/null (disabled: false)'],
['Cron job: 0 5,2 * * * hostname > /dev/null (disabled: false)'],
['Rendered template test1.txt.j2 to /tmp/test1.txt', 'Rendered template test2.txt.j2 to /tmp/test2.txt'],
['Rendered template test3.txt.j2 to /tmp/test3.txt'],
['Copy test4.txt to /tmp/test4.txt', 'Copy test5.txt to /tmp/test5.txt'],
['Copy test6.txt to /tmp/test6.txt'],
['Service chronyd started (enabled: )', 'Service firewalld started (enabled: )'],
['Service chronyd started (enabled: )']
]
actual_outputs = []
report.logs.each do |log|
actual_outputs << ansible_module_message(log)
end
assert_equal expected_outputs, actual_outputs
end
end

0 comments on commit cf4ab45

Please sign in to comment.