redhat-cop / infra.leapp

Collection of Ansible roles for automating RHEL in-place upgrades using Leapp.
MIT License
44 stars 34 forks source link

Add remediate role to collection #150

Closed Monnte closed 4 months ago

Monnte commented 7 months ago

OAMG-10379

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

Monnte commented 7 months ago

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

Monnte commented 7 months ago

@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 commented 7 months ago

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

Monnte commented 6 months ago

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

Monnte commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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

Monnte commented 5 months ago

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

djdanielsson commented 4 months ago

should we consider this a major change?

swapdisk commented 4 months ago

should we consider this a major change?

I think yes.