refactor: Refactor remediations - extract common code into role main - do not use fail/rescue#300
Conversation
|
@amott-rh https://github.com/redhat-cop/infra.leapp/pull/300/files#diff-1788d33239b6df3604a4164252101a7700ac2aa672847c342d39992157f11b19R149 - not sure if rhel6 is relevant anymore @Monnte or @bontreger https://github.com/redhat-cop/infra.leapp/pull/300/files#diff-a3d91dba9d061cfe847d2472205b2352f875c6bf7c9358d4cdd2cc1f149288c6R37 - can we remove the check for systemd mounts? I have tested this with very few actual remediations - we need to have a way, in our testing, to configure the system beforehand needing all of the remediations. I have run tests on el8 and el9 with all remediations enabled and verified that they are run correctly, or are correctly skipped. |
richm
force-pushed
the
refactor-remediations
branch
from
ba692dc to
42d1308
Compare
|
[citest] |
|
@richm Hello, I am no longer working at RedHat and not maintaining this stuff, so I can't help you with that answer. |
|
@richm Probably in general RHEL 6 has become less relevant, but I wrote that remediation precisely because I had customer engagements that were upgrading RHEL 6 servers and ran into the /usr issue. I also have colleagues in the same position, but agree it's generally rare. |
| block: | ||
| - name: leapp_cgroups-v1_enabled | Show the remediation command | ||
| ansible.builtin.debug: | ||
| msg: "{{ leapp_inhibitor_remediation.context | join(' ') }}" |
There was a problem hiding this comment.
| msg: "{{ leapp_inhibitor_remediation.context | join(' ') }}" | |
| bar: leapp_inhibitor_remediation.context | join(' ') |
| loop: | ||
| - "@^graphical-server-environment" | ||
| - gnome-desktop3 | ||
| when: kde_install.stdout | length > 0 and gnome_install.stdout | length == 0 |
There was a problem hiding this comment.
Here the role can run package_facts, and then read if packages are installed from the facts returned.
There was a problem hiding this comment.
I'm not sure about this. I don't know if the package names kde-workspace and gnome-desktop3 are aliases - the rpm command can resolve aliases, but package_facts does not list the aliases as separate keys. I would rather leave this as-is.
|
|
||
| # The old version passed the list directly to the command. I didn't think | ||
| # this was possible. Guard against something weird here. Assume non-string | ||
| # is a list. |
There was a problem hiding this comment.
@PeterMocary do you know where leapp inhibitors are stored? Maybe there is some KCS about this inhibitor that can provide us more info
richm
force-pushed
the
refactor-remediations
branch
from
6a9dbf1 to
40aa225
Compare
|
[citest] |
|
@spetrosi are these 7to8 and 9to10 errors expected? |
richm
force-pushed
the
refactor-remediations
branch
from
6c050bf to
112f715
Compare
…- do not use fail/rescue There was a lot of code duplication in the remedation task files. This has been extracted into the remediation role main task file. The remediation tasks were using fail/rescue for flow control, which makes looking at the logs very confusing, as you really have to know which tasks to exclude when looking for errors. Instead, use plain old conditionals with blocks for tasks to run conditionally. NOTE: The old way of using `rescue` would essentially catch and ignore all errors - this seems wrong to me, but perhaps there is a reason. This means that now, remediation may raise errors where before it would not. Use ansible good practices: * Use `vars` instead of `set_fact` and keep the variables set at the lowest necessary scope * Use filters instead of loops * Use conditional evaluation instead of comparing a value `== true` * Use tests such as `is success` rather than looking at the register rc value directly Signed-off-by: Rich Megginson <rmeggins@redhat.com>
richm
force-pushed
the
refactor-remediations
branch
from
112f715 to
c0fea5a
Compare
|
[citest] |
|
Any objection to merging this? We will be adding tests for remediations in upcoming PRs |
@amott-rh What was the control node platform you used, and what was the version of ansible you used? Note that I don't believe any supported version of Ansible can manage a rhel6 system (python 2.6 is too old). |
|
@richm It was a while ago, but I probably used a RHEL 7 machine running Ansible 2.9. I was working with a customer who needs to migrate a good number of RHEL 6 machines to RHEL 7, then 8 then 9, and these playbooks were the most reliable and reproducible way to achieve this. As I understand it, this project is still in progress, but I absolutely understand your point - there comes a time when old version can't be reasonably supported by them. |
4 participants
There was a lot of code duplication in the remedation task files. This has been extracted
into the remediation role main task file.
The remediation tasks were using fail/rescue for flow control, which makes looking at the
logs very confusing, as you really have to know which tasks to exclude when looking for errors.
Instead, use plain old conditionals with blocks for tasks to run conditionally. NOTE: The
old way of using
rescuewould essentially catch and ignore all errors - this seemswrong to me, but perhaps there is a reason. This means that now, remediation may raise
errors where before it would not.
Use ansible good practices:
varsinstead ofset_factand keep the variables set at the lowest necessary scope== trueis successrather than looking at the register rc value directlySigned-off-by: Rich Megginson rmeggins@redhat.com