Add remediate role to collection

[Monnte]

OAMG-10379

Introducing new role that is used to remediate the inhibitors related to the Leapp upgrade process.

[Monnte]

TODO: I need to test if everything is working as expected.

[djdanielsson]

Please fix all the lint errors

[Monnte]

@djdanielsson Hello, I am working on fixing lint errors, but, the remediation playbooks are taken from insights and plan is automatically sync them into this role, but the lint fails on those playbooks. So my question is, can we ignore those files or the other (not preferred) option is to update all remediation playbooks to pass the lint. WDYT?

[djdanielsson]

No, the lint is required. Please reach out to me offline and we can discuss this a bit more in detail

[Monnte]

@djdanielsson Hi, can you try to rerun the linter now ?

[Monnte]

Pipeline now should pass, however there are some "nasty" things that should be fixed but didn't find time yet.

There were a lot of changes to the playbooks, some visual and some functional therefore we need to test them all again.

[djdanielsson]

I have not really had time to look over the code, but if it passes lint and has been tested functionality I think I am willing to merge it @swapdisk thoughts?

[Monnte]

I need to test those changes and merge it to the upstream in internal project, but when it's ready I'll let you know. So please wait with merging the branch. Thanks.

[swapdisk]

I need to test those changes and merge it to the upstream in internal project, but when it's ready I'll let you know. So please wait with merging the branch. Thanks.

Right, let's move the PR to draft in the meantime.

I have not really had time to look over the code, but if it passes lint and has been tested functionality I think I am willing to merge it @swapdisk thoughts?

I will give all this a good look through tomorrow.

[jeffmcutter]

I'm surprised uppercase file names pass lint. Is there a reason for them? The rest of the collection uses task files lower-lower-lower.yml, not UPPER_UPPER_UPPER.yml.

[Denney-tech]

I'm surprised uppercase file names pass lint. Is there a reason for them? The rest of the collection uses task files lower-lower-lower.yml, not UPPER_UPPER_UPPER.yml.

While that's true, as a consumer I don't look at any of the other roles and think I need to do something like:

- include_role:
    name: infra.leapp.analysis
    tasks_from: analysis-leapp.yml

I would just use the default main.yml and let the role decide what task files to run, when and where.

In this case for this role, the default tasks_from: main.yml would do absolutely nothing unless I fill the remediation_todo: [] var with an array of task files I want it to run (filenames minus the extension). With this behavior in mind, and having to provide documentation about what these do, I think UPPER_UPPER_UPPER.yml is acceptable for all task files that might be called this way. Any other task files not called this way could use the usual lower-lower-lower.yml. This would also make it easier to sanitize the remediation_todo: [] var in an assertion step or by piping it to the upper filter during the loop.

In the meantime, the following playbook would never end since the main.yml file doesn't prevent itself from calling itself or other lower-cased task files.

- include_role:
    name: infra.leapp.remediation
  vars:
    remediation_playbooks:
      - LEAPP_NFS_DETECTED
      - main
    remediation_todo:
      - LEAPP_NFS_DETECTED
      - main

[Monnte]

I think, it is ready to be merged if there are no further requirements or changes.

Followup automatization for automatic update from the upstream is ready. Whenever the change is presented in upstream, our bot will create the PR to this repo, to update the playbook.

[jeffmcutter]

@Denney-tech and @Monnte

Regarding the case. Users will have to keep track of uppercase vs. lowercase variable names.

I don't think I've ever seen another certified, validated, or well known community collection using uppercase variables and files.

As someone who has contributed a lot to this collection, I would really like to see the variable and file names remain consistent.

If it's too much of an ask, I can help rename them.

Thanks,
-Jeff

[djdanielsson]

I think something like this should do the trick for the variables that are all caps:

for i in $(find . -type f -name "*.yml");do sed -i 's/[A-Z(_)?]*\:/\L&/' "$i";done

and for the file names I think this should work but untested:

for i in $(find . -type f -name ".yml");do mv -v "$i" "`echo $i | tr '[A-Z]' '[a-z]'`";done

[Monnte]

One more thing, those playbooks are for RHEL 8 remediation only I think, where would be the ideal place to mention this in documentation ?

[djdanielsson]

few small tweaks. I would put it at the top of the readme in the role itself and note it in the main readme as well where you added the line.

[Monnte]

@djdanielsson thanks, for now I will adjust all the things to mention RHEL 8, until remediation for other version will be available.

[djdanielsson]

LGTM

[swapdisk]

After fixing the loop var, I was able to get further. I ran the role with using this just for testing:

remediation_todo:
  - leapp_missing_pkg

The role failed at the "Install the missing package via remediation command" task like this:

TASK [infra.leapp.remediate : leapp_missing_pkg | Install the missing package via remediation command] ***
fatal: [more-calf]: FAILED! => {"msg": "The task includes an option with an undefined variable. The error was: 'str object' has no attribute 'context'. 'str object' has no attribute 'context'\\n\\nThe error appears to be in '/runner/requirements_collections/ansible_collections/infra/leapp/roles/remediate/tasks/leapp_missing_pkg.yml': line 31, column 7, but may\\nbe elsewhere in the file depending on the exact syntax problem.\\n\\nThe offending line appears to be:\\n\\n\\n    - name: leapp_missing_pkg | Install the missing package via remediation command\\n      ^ here\\n"}

It appears the remediation playbook expects to always find its corresponding inhibitors title in the leapp report file.

In the name of idempotency, may I suggest we make it so that remediation playbooks just gracefully do nothing if the corresponding inhibitor was not reported?

[swapdisk]

Bad sed! 😄

[swapdisk]

LGTM, but please add a change description under changelogs/fragments/.

[djdanielsson]

should we consider this a major change?

[swapdisk]

should we consider this a major change?

I think yes.

[swapdisk]

LGTM.