-
Notifications
You must be signed in to change notification settings - Fork 70
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
Change output format to report overview #840
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
To launch regression testing public members of oamg organization can leave the following comment:
Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please consider rerunning the CI by commenting leapp-ci build (might require several comments). If the problem persists, contact leapp-infra. |
Requires #838 |
Leapp introduced new output apis (and removed some other). See oamg/leapp#840 for details.
/rerun 1116 |
Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/6311627 |
Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/6311680 |
Testing Farm request for RHEL-8.6-rhui/6311680;6311627 regression testing has been created. |
Testing Farm request for RHEL-7.9-rhui/6311680;6311627 regression testing has been created. |
e640bcb
to
0fb569e
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.
lgtm! The code looks good, did not find any major issues nor obvious bugs. I ran upgrade and preupgrade and simulated different inhibitors and errors and the output looks at advertised
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 need to do some testing yet, as it's kinda late, I am possibly overlook something and my comments could be wrong. Let's sync in the morning how we will handle this.
leapp/reporting/__init__.py
Outdated
# Before removing Flags, the original inhibitor detection worked | ||
# by checking the `flags` field; keep the `flags` field until we drop Flags completely | ||
# Currently we know that 'flags' does not exist otherwise so it's | ||
# safe to just set it | ||
report['flags'] = [Groups.INHIBITOR] | ||
if Groups.ERROR in report.get('groups', []): | ||
# see above for explanation | ||
report['flags'] = [Groups.ERROR] |
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.
@matejmatuska @dkubek @fernflower I've realized this is problematic, as the list of flags is specified in the report-schema-v110.json. so the json file cannot contain the flag. to save a time, when report v110 is generated, ignore the error flag (in the way it's not present in the json file). it's compatible however with v120, as groups
are not enumerated.
we could introduce report v111, however, I am afraid we cannot set it as default anyway as I am not sure how it will be accepted by plugins for cockpit, insights, and satellite/foreman.
@fernflower wdyt? could we do a fast check in the morning whether all mentioned plugins are ok with the new flag, so we could introduce report v111 and set it as default?
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.
also, we could keep error as a tag, but it does not fit there well. let's discuss that in the morning
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.
thinking about that after the sleep, I vote to go the most safe way. We will drop it from the json file completely and it will be covered properly the next time.
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.
In this case it shows up as in the 1.2.0 schema:
"groups": [
"error"
],
And not flags/tags field.
This should be the problematic line:
https://github.com/oamg/leapp/pull/840/files#diff-a0dcff5a59a744244ee3056a16cd7fca4bbcc6cd573c6d6e9dd26ddb4205723eL151
84c7dea
to
6cdb2c3
Compare
Leapp introduced new output apis (and removed some other). See oamg/leapp#840 for details. The calls to `generate_report_file()` in `commands/upgrade/utils.py` are swapped to workaround a bug in the framework where the json report generation modifies messages passed to it. Jira: OAMG-9663
Currently there is no straightforward way to identify whether a report is an ordinary report or an error report (report generated from an error). An `ERROR` group is introduced in `leapp.reporting.Groups.ERROR` which is used to group error reports. This is utilized in the leapp output to show the number of error reports in the summary. Also error reports in `leapp-report.txt` are now followed by a "(error)" string similar to inhibitors. For now ERROR isn't in flags nor tags in the leapp-report.json in report schema 1.1.0. This because integration with plugins might break, due to the enumeration of possible flags in the schema. This will be adjusted later. In report schema 1.2.0 the ERROR group is listed in groups as expected. Jira: OAMG-9532
This patch improves the leapp output format. The "REPORT" section is renamed to "REPORT OVERVIEW" and in addition to high and medium priority report titles it contains also inhibitor titles and actor and short error message for errors. The long (possibly thousands of lines) detailed errors messaged and tracebacks are kept in the "ERRORS" section. The inhibitors section is removed, because it's contents are now in the report overview. Jira: OAMG-9663
6cdb2c3
to
4b0d48c
Compare
Tested manually, works as expected, see below. error
For the schema v1.2.0:
Snippet from the json file
The rest of entries looks good as I expected too. I forgot to put here inhibitors (will try to get outputs also later) but it seemed correct to me also. Here is one positive:
|
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.
see my previous comments. works as expected. merging
Leapp introduced new output apis (and removed some other). See oamg/leapp#840 for details. The calls to `generate_report_file()` in `commands/upgrade/utils.py` are swapped to workaround a bug in the framework where the json report generation modifies messages passed to it. Jira: OAMG-9663
## Packaging - Provide leapp-framework 5.0 (oamg#818, oamg#840) ## Framework ### Fixes - Improve processing of reports with UTF-8 characters (oamg#821, oamg#828) - Fix info reporting with only one path to log (oamg#834) ### Enhancements - Include tracebacks of actors into the leapp.db (oamg#832) ## Leapp (tool) ### Fixes - Dialog creation fails not more for a component without choices (oamg#826) - Empty report is generated correctly (oamg#828) - Fix processing data in remediation instructions with non-ascii characters () ### Enhancements - Improve report summary output to make it more visible (oamg#818, oamg#840) ## stdlib ### Fixes - Fixed the call when the execute cannot be performed (oamg#836) ### Enhancements - changes related just to stdlib - under leapp/libraries/stdlib
## Packaging - Provide leapp-framework 5.0 (oamg#818, oamg#840) ## Framework ### Fixes - Improve processing of reports with UTF-8 characters (oamg#821, oamg#828) - Fix info reporting with only one path to log (oamg#834) ### Enhancements - Include tracebacks of actors into the leapp.db (oamg#832) ## Leapp (tool) ### Fixes - Dialog creation fails not more for a component without choices (oamg#826) - Empty report is generated correctly (oamg#828) - Fix processing data in remediation instructions with non-ascii characters () ### Enhancements - Improve report summary output to make it more visible (oamg#818, oamg#840) ## stdlib ### Fixes - Fixed the call when the execute cannot be performed (oamg#836) ### Enhancements - changes related just to stdlib - under leapp/libraries/stdlib
## Packaging - Provide leapp-framework 5.0 (#818, #840) ## Framework ### Fixes - Improve processing of reports with UTF-8 characters (#821, #828) - Fix info reporting with only one path to log (#834) ### Enhancements - Include tracebacks of actors into the leapp.db (#832) ## Leapp (tool) ### Fixes - Dialog creation fails not more for a component without choices (#826) - Empty report is generated correctly (#828) - Fix processing data in remediation instructions with non-ascii characters () ### Enhancements - Improve report summary output to make it more visible (#818, #840) ## stdlib ### Fixes - Fixed the call when the execute cannot be performed (#836) ### Enhancements - changes related just to stdlib - under leapp/libraries/stdlib
This patch improves the leapp output format. The "REPORT" section is
renamed to "REPORT OVERVIEW" and in addition to high and medium priority
report titles it contains also inhibitor titles and actor and short
error message for errors.
The long (possibly thousands of lines) detailed errors messaged and
tracebacks are kept in the "ERRORS" section. The inhibitors section is
removed, because it's contents are now in the report overview.