-
Notifications
You must be signed in to change notification settings - Fork 84
[RHELC-1761] Format messages for consistency #1438
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1438 +/- ##
==========================================
+ Coverage 96.11% 96.29% +0.17%
==========================================
Files 72 72
Lines 5176 5184 +8
Branches 895 896 +1
==========================================
+ Hits 4975 4992 +17
+ Misses 119 115 -4
+ Partials 82 77 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
convert2rhel/unit_tests/actions/system_checks/tainted_kmods_test.py
Outdated
Show resolved
Hide resolved
5457d34 to
1fd0e5b
Compare
|
Lots of merge conflicts now 😬 |
hosekadam
left a comment
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.
Some minor comments, but overall looks good
e5b5f5c to
c2c5673
Compare
|
Conflicts resolved. |
adc6f72 to
6bd4220
Compare
6bd4220 to
dbce386
Compare
|
@hosekadam, @Venefilyn, @danmyway, ready for review. |
dbce386 to
d0fd418
Compare
60e471d to
4846937
Compare
|
/packit test |
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.
Didn't get through most of the code yet but see some things to change about wording, dots inconsistencies.
Most importantly though, lets not change the IDs unless we plan on a major version bump.
| loggerinst.task("List remaining non-Red Hat packages") | ||
|
|
||
| loggerinst.info("Listing packages not signed by Red Hat") | ||
| loggerinst.info("Listing packages not signed by Red Hat.") |
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.
This will be unique as most don't have a dot in them, but definitely more valid with a dot than without. Just an FYI
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.
Most info messages had a full stop. For consistency I've added the full stop to all of them (and to other levels such as debug or critical as well).
| level="INFO", | ||
| id="SKIPPED_MODIFIED_RPM_FILES_DIFF", | ||
| title="Skipped comparison of 'rpm -Va' output from before and after the conversion.", | ||
| title="Skipped comparison of 'rpm -Va' output from before and after the conversion", |
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.
Dot removed here as well
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.
This is intentional. It's one of the goals of the PR - to remove full stops from message titles for consistency. Most titles didn't have it, so I opted for removing them from all.
The message titles are printed this way:
(INFO) MODIFIED_RPM_FILES_DIFF::FOUND_MODIFIED_RPM_FILES - Modified rpm files from before and after the conversion were found
Description: ...
In Insights they are displayed in this fashion:

In both cases (CLI, Insights) it's more suitable to have the titles without full stops.
| """Back up redhat release file before starting conversion process""" | ||
| logger.task("Back up redhat-release files") | ||
|
|
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 would say Backup instead of back up. What is the Red Hat preferred word?
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.
Backup is a noun, back up is a verb.
"You are backing up a file and then you have a file backup."
4846937 to
97b635d
Compare
|
/packit test |
97b635d to
1d1061b
Compare
|
/packit test |
1d1061b to
63f5bbb
Compare
|
/packit test |
Venefilyn
left a comment
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! One small comment but fine to merge
db59d82 to
dddca52
Compare
- no full stop at the end of a report message title - no word please (technical writers' rule) - boot loader => bootloader - fix typos - add missing context where messages are too short and vague - make TASK tense consistent - ensure each Action has a TASK level log message - ensure every Action prints what it's done - add unit tests to lines reported by caplog as uncovered
dddca52 to
d1ead6c
Compare
|
/packit test |
2 similar comments
|
/packit test |
|
/packit test |
unique report message idsJira Issues:
Checklist
[RHELC-]or[HMS-]is part of the PR titleRelease Pendingif relevant