Skip to content
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-1347] Update report to include post conversion actions #1151

Merged
merged 4 commits into from
Apr 24, 2024

Conversation

r0x0d
Copy link
Member

@r0x0d r0x0d commented Mar 19, 2024

The report had to be updated to include the post conversion results from
the actions, since we are extending the framework and migrating the
functions to become actions, we need to make a few modifications to
adapt the reporting to work with both pre and post conversion actions.

Jira Issues: RHELC-1347

Checklist

  • PR has been tested manually in a VM (either author or reviewer)
  • Jira issue has been made public if possible
  • [RHELC-] is part of the PR title
  • GitHub label has been added to help with Release notes
  • PR title explains the change from the user's point of view
  • Code and tests are documented properly
  • The commits are squashed to as few commits as possible (without losing data)
  • When merged: Jira issue has been updated to Release Pending if relevant

Depends on

@r0x0d r0x0d force-pushed the modify-post-conversion-report branch from 5f09650 to 1292d73 Compare March 19, 2024 18:40
@r0x0d r0x0d force-pushed the modify-post-conversion-report branch from 1292d73 to 47a0dc7 Compare March 22, 2024 17:30
@r0x0d r0x0d marked this pull request as ready for review March 22, 2024 17:30
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 97.29730% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 95.38%. Comparing base (2644404) to head (7633a3a).

Files Patch % Lines
convert2rhel/main.py 95.45% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1151      +/-   ##
==========================================
+ Coverage   95.36%   95.38%   +0.01%     
==========================================
  Files          53       53              
  Lines        4619     4635      +16     
  Branches      809      810       +1     
==========================================
+ Hits         4405     4421      +16     
  Misses        137      137              
  Partials       77       77              
Flag Coverage Δ
centos-linux-7 90.48% <97.29%> (+0.03%) ⬆️
centos-linux-8 91.46% <97.29%> (+0.02%) ⬆️
centos-linux-9 91.51% <94.59%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@r0x0d r0x0d force-pushed the modify-post-conversion-report branch from 47a0dc7 to f46d9c7 Compare March 22, 2024 18:57
convert2rhel/actions/report.py Outdated Show resolved Hide resolved
@r0x0d r0x0d force-pushed the modify-post-conversion-report branch from f46d9c7 to fc933d7 Compare March 26, 2024 18:11
Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't review unit tests yet

convert2rhel/main.py Outdated Show resolved Hide resolved
convert2rhel/main.py Show resolved Hide resolved
@r0x0d r0x0d force-pushed the modify-post-conversion-report branch 2 times, most recently from 6fbf2af to e21d561 Compare April 8, 2024 17:07
@r0x0d r0x0d added kind/feature New feature or request tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. labels Apr 10, 2024
@has-bot
Copy link
Member

has-bot commented Apr 10, 2024

/packit test --labels tier0


Comment generated by an automation.

@r0x0d r0x0d force-pushed the modify-post-conversion-report branch from e21d561 to 97a67f7 Compare April 11, 2024 16:54
@r0x0d r0x0d requested a review from Venefilyn April 11, 2024 16:55
@r0x0d
Copy link
Member Author

r0x0d commented Apr 15, 2024

/packit test --labels tier0

@r0x0d r0x0d force-pushed the modify-post-conversion-report branch from 97a67f7 to 78fc118 Compare April 16, 2024 16:26
Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation seems pretty good to me, just wondering about the usage of deciding pre-conversion vs post-conversion report in the try-finally. I feel like it introduces technical debt for future refactoring

@r0x0d
Copy link
Member Author

r0x0d commented Apr 17, 2024

Implementation seems pretty good to me, just wondering about the usage of deciding pre-conversion vs post-conversion report in the try-finally. I feel like it introduces technical debt for future refactoring

@SpyTec, I agree, and by far, this doesn't even look like a solid implementation.

What do you think about something like this? (It's just a draft, so I just put whatever name I first thought of)

diff --git a/convert2rhel/main.py b/convert2rhel/main.py
index f59b738..f26e563 100644
--- a/convert2rhel/main.py
+++ b/convert2rhel/main.py
@@ -43,6 +43,18 @@ class ConversionPhase:
     POST_PONR_CHANGES = 4
 
 
+_REPORT_MAPPING = {
+    ConversionPhase.PRE_PONR_CHANGES: (
+        report.CONVERT2RHEL_PRE_CONVERSION_JSON_RESULTS,
+        report.CONVERT2RHEL_PRE_CONVERSION_TXT_RESULTS,
+    ),
+    ConversionPhase.POST_PONR_CHANGES: (
+        report.CONVERT2RHEL_POST_CONVERSION_JSON_RESULTS,
+        report.CONVERT2RHEL_POST_CONVERSION_TXT_RESULTS,
+    ),
+}
+
+
 def initialize_file_logging(log_name, log_dir):
     """
     Archive existing file logs and setup all logging handlers that require
@@ -182,16 +194,12 @@ def main_locked():
         # Write the assessment to a file as json data so that other tools can
         # parse and act upon it.
         results = _pick_conversion_results(process_phase, pre_conversion_results, post_conversion_results)
-        json_filepath = report.CONVERT2RHEL_PRE_CONVERSION_JSON_RESULTS
-        txt_filepath = report.CONVERT2RHEL_PRE_CONVERSION_TXT_RESULTS
 
-        if process_phase == ConversionPhase.POST_PONR_CHANGES:
-            json_filepath = report.CONVERT2RHEL_POST_CONVERSION_JSON_RESULTS
-            txt_filepath = report.CONVERT2RHEL_POST_CONVERSION_TXT_RESULTS
+        if results and process_phase in _REPORT_MAPPING:
+            json_report, txt_report = _REPORT_MAPPING[process_phase]
 
-        if results:
-            report.summary_as_json(results, json_filepath)
-            report.summary_as_txt(results, txt_filepath)
+            report.summary_as_json(results, json_report)
+            report.summary_as_txt(results, txt_report)
 
     return 0

It looks less bad or more bad?

@Venefilyn
Copy link
Member

It looks less bad or more bad?

I think it definitely looks better. If you can change it to that for now that would be good, then we can merge. For _pick_conversion_results I still feel like that function has a bad smell but something we can take care of in the future if we ever get to an issue there that requires refactoring

@r0x0d r0x0d force-pushed the modify-post-conversion-report branch from 78fc118 to e18a0f6 Compare April 18, 2024 12:21
In this commit, we are introducing two new stages for enabling the post
conversion in the action framework.

We are also introducing an environment variable to not break the current
workflow and let the conversion function as-is for now. Anyone that is
willing to test and run the post conversion actions will need to use
this environment var.
The report had to be updated to include the post conversion results from
the actions, since we are extending the framework and migrating the
functions to become actions, we need to make a few modifications to
adapt the reporting to work with both pre and post-conversion actions.
Introducing a mapping that handles the correlation between conversion
phase and path of json/txt files.
@r0x0d r0x0d force-pushed the modify-post-conversion-report branch from e18a0f6 to 7633a3a Compare April 23, 2024 12:44
@r0x0d
Copy link
Member Author

r0x0d commented Apr 23, 2024

/packit test --labels tier0

@Venefilyn Venefilyn merged commit fde8bc9 into oamg:main Apr 24, 2024
33 checks passed
@hosekadam hosekadam mentioned this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants