-
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-1332] Port PkgManagerConf() to the Action framework #1321
[RHELC-1332] Port PkgManagerConf() to the Action framework #1321
Conversation
345be37
to
0fd5623
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1321 +/- ##
==========================================
+ Coverage 96.30% 96.35% +0.05%
==========================================
Files 59 60 +1
Lines 4872 4884 +12
Branches 858 860 +2
==========================================
+ Hits 4692 4706 +14
+ Misses 103 101 -2
Partials 77 77
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
convert2rhel/unit_tests/actions/conversion/pkg_manager_config_test.py
Fixed
Show resolved
Hide resolved
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.
Looks good to me.
The only thing is that the action looks confusing right now, as the name of the class is almost the name of what we call underneath it. The only main difference in the naming is the ig
at the name of the action class 🤔
Is there a way we can improve it somehow?
@r0x0d I could extend the name of the action class to PackageManagerConfiguration or maybe ConfigurePkgManager |
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 changes necessary
ba6275b
to
728a723
Compare
/packit test --labels sanity |
eb03a7b
to
2538ea6
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.
Looks good!
/packit build |
1 similar comment
/packit build |
/packit test --labels sanity |
/packit retest-failed |
Failing int tests are because we have updated the patch file that gets asserted to be yum/dnf based on the pkg manager. Before the patch file that got output was always yum.conf |
/packit test --labels sanity |
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.
Just needs to be rebased
Co-authored-by: Adam Hošek <hosek.adam@outlook.com>
for more information, see https://pre-commit.ci
Co-authored-by: Freya Gustavsson <freya@spytec.se> Co-authored-by: Rodolfo Olivieri <rodolfo.olivieri3@gmail.com>
Co-authored-by: Rodolfo Olivieri <rodolfo.olivieri3@gmail.com>
ede4a52
to
c0b3908
Compare
@@ -9,6 +9,9 @@ def test_yum_conf_patch(convert2rhel, shell): | |||
expanding the $releasever variable properly. | |||
""" | |||
shell("echo '#random text' >> /etc/yum.conf") |
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 shell command should be placed little down, once we know which config we want to edit (either the dnf or yum conf)
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 doesn't need to since, /etc/yum.conf
is either a config file or a symlink to /etc/dnf/dnf.conf
A comment would be nice though
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 see, thanks for the info. I haven't realized this. And +1 for the comment
This PR calls the PkgManagerConf class inside a new action called pkg_manager_config so it can be executed alongside the other actions in the conversion section. Relevant tests have been written.
Jira Issues:
Checklist
[RHELC-]
or[HMS-]
is part of the PR titleRelease Pending
if relevant