Add the parameter ansible_roles_diff_mode to enable Ansible diff mode #749
Conversation
|
Any update? |
|
Hello @rpluem-vf Just so you're aware I've submitted #764 to add diff mode support to Job Templates. |
Hi, it may take some time before we're able to start reviewing this PR. Thank you for your patience and understanding. |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
Overall I think this reads well. I haven't tested it myself yet. Some minor notes inline.
Just a general note that all the translations in this helper are broken (because you can't interpolate strings this way), but they already were before you came in.
| have_diff = false | ||
| get_results(msg_json) do |_, result| | ||
| if result['report_diff'].present? | ||
| have_diff = true | ||
| break | ||
| end | ||
| end | ||
| if have_diff | ||
| get_results(msg_json) do |module_args, result| | ||
| add_diff('', module_args['path'] || '', result) if result['report_diff'].present? | ||
| end | ||
| else | ||
| no_data_message | ||
| end |
There was a problem hiding this comment.
Can't you merge these 2 together into a single loop?
There was a problem hiding this comment.
I don't see the change. I expected a single get_results call
There was a problem hiding this comment.
My bad. I did not add this to the commit. Should be there now.
|
Added the suggestion based on the rubocop output, rebased and squashed the commits. |
|
@ekohl Can you please approve the workflows such that we can move on? |
|
docs triage: @nofaralfasi Could you please review this PR? |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
The code looks good to me, though I haven't found the time to actually test this out myself. One minor note inline.
…sible diff mode Add the parameter ansible_roles_diff_mode to enable Ansible diff mode. Furthermore add the needed functionality to the reports helper to show these diffs in reports. * app/helpers/foreman_ansible/ansible_reports_helper.rb: Check if the result hash of a task contains a 'report_diff' key and make its value, the unified diff, available via a link. * app/models/foreman_ansible/ansible_provider.rb: Set the diff_mode smart proxy parameter based on the parameter ansible_roles_diff_mode. * db/seeds.d/100_common_parameters.rb: Create the global parameter ansible_roles_diff_mode with a default of false. * test/fixtures/report.json: Add fixture data for new test case * test/unit/helpers/ansible_reports_helper_test.rb: - Adjust expected strings existing test cases - Add new test case for diff_mode - Include further modules used by ansible_reports_helper.rb Signed-off-by: Ruediger Pluem <[email protected]> Improve hash lookup in app/helpers/foreman_ansible/ansible_reports_helper.rb Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]> Fix rubocop errors in app/helpers/foreman_ansible/ansible_reports_helper.rb Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]> Add trailing space to translated message Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
|
Now the workflows wait for approval again |
|
@adamruzicka Please run the GHA. |
|
Is there anything that could be done to move this forward? |
|
@ekohl : Any update on this PR? |
|
Any update on this PR? |
5 participants
Overview of Changes
ansible_roles_diff_modeto enable Ansible diff mode. Furthermore add the needed functionality to the reports helper to show these diffs in reports.Implementation Considerations
Testing Steps
ansible_roles_diff_modeparameter to true for this host. Run the AnsibleAnsible Roles - Ansible Defaultjob. Afterwards the diff of the change can been seen in the script output and in the config report via a link. This requires all three PR's.Checklists
Additional Notes
Depends on the following PR's: