Add 'report_diff' key in result data in case of available diff data #1816
Conversation
|
Why would you alter the data here instead of passing before/after verbatim to Foreman and let the UI render it in whichever way it likes? |
This saves space. If you have a 100 KB file and change only one line you have to save 200 KB in the report (100 KB for the file before the change, 100 KB for the file after the change). If you have this for thousands of hosts and a number of larger configuration files this sums up. |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1816 +/- ##
==========================================
Coverage ? 35.36%
==========================================
Files ? 94
Lines ? 4604
Branches ? 1134
==========================================
Hits ? 1628
Misses ? 2973
Partials ? 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
UI wise this uses what is used for the Puppet data to render which only delivers the unified diffs. Plus |
|
The Ansible callback here is just "the messenger", we (well, you ;) ) can adapt to whatever foreman/foreman_ansible (I don't recall which one carries the report parsing these days) expects. If it's easier (and cheaper) to follow the Puppet path, so be it. BTW, I always thought that Ansible already provided a unified diff in the |
I was surprised as well. I will have a look at my code again because there seems to be a function already for callbacks: and it is used here and here. |
|
I have two further PR's that are unrelated to this new feature, but fix issues that cause tests to fail: @evgeni Can you have a look? Once they are in I would rebase this PR. Furthermore I rewrote the code to use the function I mentioned above to generate the diff instead of my own. It is more generic and covers cases that I did not consider so far. |
|
PR's for this feature in other repositories: theforeman/smart_proxy_ansible#98 |
|
I have merged the test cleanup PRs, thanks for that! The diff here looks quite reasonable, but I guess we should first get movement on the receiving end (foreman_ansible) for proceeding further? |
As promised I just pushed a rebased commit with tests that makes use of the existing function
I think there is no hard dependency. If the updated callback runs against an older Foreman there are just no diffs in the data coming from Ansible. And even if there are as I added a new key |
|
I see that there are failures currently. I will take care of them and then you can have a look. Stay tuned. |
rpluem-vf
force-pushed
the
diff_mode
branch
5 times, most recently
from
2cd1b97 to
c6d428d
Compare
|
@evgeni : Now all is green :-) |
|
Any update? |
|
I think the diff looks fine, but I'd prefer theforeman/smart_proxy_ansible#98 and theforeman/foreman_ansible#749 being merged first. |
Understood. Any idea how we can push these to get merged? |
|
The last push was just a rebase that solved a conflict. |
In case that a 'diff' key with 'before' and 'after' data is available
create a unified diff from 'before' and 'after' and store it below the
'report_diff' key as a string for later consumption in config reports
on the Foreman server.
Remove the 'diff' key afterwards to save space as in case of a file the
content is probably big and it is stored twice (before and after).
* plugins/callback/foreman.py:
Implement feature.
* tests/callback/three_hosts.yml:
Add tasks to generate diffs when running in diff mode.
* tests/conftest.py:
Allow to run the playbook in diff mode.
* tests/test_callback.py:
- Run the playbook for the foreman report type in diff mode
- Ignore further items when comparing the json data that depend on the
local environment
- Remove additional strings that are different in each run
- Transform the json string into json data to improve data handling
* tests/fixtures/callback/dir_store/*:
Adjust fixtures to new tests and changes in format.
Signed-off-by: Ruediger Pluem <[email protected]>
|
The last push was just a rebase that solved a conflict in tests/test_callback.py. |
3 participants
Enrich the data for config reports with unified diffs in case the Ansible tasks provides diff data. This is part of a series of PR's to enable the Ansible diff mode on Foreman.