-
Notifications
You must be signed in to change notification settings - Fork 84
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
[RHELC-1193] Identify highest status in json report #983
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #983 +/- ##
==========================================
- Coverage 94.20% 94.18% -0.03%
==========================================
Files 47 47
Lines 4368 4383 +15
Branches 774 779 +5
==========================================
+ Hits 4115 4128 +13
Misses 177 177
- Partials 76 78 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
assert result == expected | ||
|
||
|
||
def test_find_highest_report_level_unknown_status(): |
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.
Maybe I would use the pytest.mark.parametrize
for easier addition of new cases. But question is if that would be ever needed.
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.
Likely won't have new cases. Just having FOO as an unknown status satisfies checking that unknown statuses will not be added to our output
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.
agreed with @pr-watson. Can't see a case where we would need to parametrize this.
ffa4686
to
9874d35
Compare
/packit retest-failed |
schemas/assessment-schema-1.1.json
Outdated
@@ -0,0 +1,150 @@ | |||
{ | |||
"$schema": "https://json-schema.org/draft/2020-12/schema", | |||
"$id": "https://raw.githubusercontent.com/oamg/convert2rhel/main/schemas/assessment-schema-1.0.json", |
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.
"$id": "https://raw.githubusercontent.com/oamg/convert2rhel/main/schemas/assessment-schema-1.0.json", | |
"$id": "https://raw.githubusercontent.com/oamg/convert2rhel/main/schemas/assessment-schema-1.1.json", |
# Use an envelope so we can add other, non-result info if necessary. | ||
envelope = { | ||
"format_version": "1.0", | ||
"format_version": "1.1", |
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.
Why do we add the format_version
here and not using the one in the schema file?
55823f0
to
64f73d6
Compare
convert2rhel/actions/report.py
Outdated
they come. | ||
""" |
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.
Please, if you can, update the docstring to include the param/type/return/rtype.
they come. | |
""" | |
they come. | |
:param results: ... | |
:type results: ... | |
:return: ... | |
:rtype: ... | |
""" |
schemas/assessment-schema-1.1.json
Outdated
"const": "1.0" | ||
}, | ||
"status": { | ||
"description": "The highest severity of all actions.", |
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.
Maybe not necessary, but I would include "between messages and results" in this description
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.
It looks good to me. Just left a couple of questions and I think one of @SpyTec comments is necessary to apply. Not approving it yet, but I can see the PR is correct.
@r0x0d just added the requested suggestions, ready for another round of review |
64f73d6
to
f4c9bff
Compare
/packit test |
/packit build |
/packit retest-failed |
/packit test |
1 similar comment
/packit test |
/packit test --labels sanity |
1 similar comment
/packit test --labels sanity |
f4c9bff
to
6d280fe
Compare
/packit test --labels sanity |
This PR adds a function find_highest_report_level to report.py to get the highest status level within the results and messages reported in the analysis. This highest status level is added as a new key to the report json at the top level for use in the rhc worker script.
Jira Issues: RHELC-1193
Checklist
[RHELC-]
is part of the PR titleRelease Pending
if relevant