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.

Sign up for GitHub

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

Jump to bottom

Extend LEAPP with actor configuration #870

Merged
merged 3 commits into from
Nov 13, 2024
Merged

Extend LEAPP with actor configuration #870

merged 3 commits into from
Nov 13, 2024

Conversation

abadger
Copy link
Member

@abadger abadger commented Jul 2, 2024 •
edited by MichalHe
Loading

Provide a new unified solution to enable User-Configurable actors.

The point is to introduce mechanism how various parts of workflow could be configured, on level actors which should be able to easily access configuration related to them (e.g. via a framework API if designed in such a way). So users will not have to create their own custom actors for all possibilities we provide right now, but they could simply create/update configuration files to achieve what they want.

Issues:

  • Reading configs on RHEL7 (Python 2.7) fails, crashing the entire framework because glob.glob has no kw argument recursive=True

JIRA: OAMG-8803

@abadger abadger marked this pull request as draft July 2, 2024 08:01
@abadger abadger mentioned this pull request Jul 2, 2024
Merged
5 tasks
@pirat89 pirat89 added this to the 8.10/9.5 milestone Jul 2, 2024
@pirat89 pirat89 modified the milestones: 8.10/9.5, 8.10/9.6 Aug 16, 2024
@abadger abadger force-pushed the actor-config branch 9 times, most recently from 3f26947 to b1f67db Compare September 13, 2024 08:52
@dkubek
Copy link
Member

dkubek commented Sep 16, 2024

Adding quick note here so we won't forget. We will want to include the config information also in leapp.db. If we make the Config serializable we could simply edit store_actor_metadata in leapp/utils/audit/__init__.py and it should work well enough.

dkubek
dkubek reviewed Sep 16, 2024
View reviewed changes
leapp/actors/__init__.py Outdated Show resolved Hide resolved
@abadger abadger force-pushed the actor-config branch 5 times, most recently from 1f75fe5 to bf68773 Compare September 19, 2024 09:19
@abadger
Copy link
Member Author

abadger commented Sep 19, 2024

Hey @vinzenz , I don't know if you have time but I was wondering if you recall where in the code I need to add something to recognize directories inside of the actor as being a plugin? I'm trying to add a configs/ directory that is pretty much the same as libraries/. I found the places in the code I need to modify to make a configs directory at the repository level work but I'm stll not recognizing a configs directory inside of each individual actor. I know I'm probably just missing a few line of code somewhere that sets it up but I can't seem to find it.

@dkubek dkubek changed the title Start figuring out where to add configuration Extend LEAPP with actor configuration Oct 10, 2024
@MichalHe
Copy link
Member

/packit build

MichalHe
MichalHe requested changes Oct 29, 2024
View reviewed changes
Copy link
Member

@MichalHe MichalHe left a comment

Choose a reason for hiding this comment

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

I went through the code. It looks OK, but I cannot claim I am sure since the codebase is a bit hidden in "fog of war". @dkubek I will take care of the my own review suggestions. Can you comment on the comment where I tagged you explicitly?

leapp/actors/__init__.py Outdated Show resolved Hide resolved
leapp/actors/__init__.py Outdated Show resolved Hide resolved
leapp/actors/config.py Outdated Show resolved Hide resolved
leapp/actors/config.py Outdated Show resolved Hide resolved
leapp/actors/config.py Outdated Show resolved Hide resolved
leapp/actors/config.py Outdated Show resolved Hide resolved
leapp/configs/common/__init__.py Outdated Show resolved Hide resolved
leapp/models/fields/__init__.py Outdated Show resolved Hide resolved
leapp/repository/actor_definition.py Show resolved Hide resolved
leapp/utils/audit/__init__.py Outdated Show resolved Hide resolved
@dkubek dkubek marked this pull request as ready for review October 31, 2024 12:54
pirat89
pirat89 reviewed Nov 11, 2024
View reviewed changes
Copy link
Member

@pirat89 pirat89 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In general I think it's ok. I haven't found any serious problems in the code that would not be already covered and some stuff that I think we should still deliver is covered by TODOs which can definitely wait for a follow up PR. However, when I've been testing configs with the RHUI configuration in leapp-repository, I found several things that seems suspicious to me, but it's possibly unrelated to the framework and it's problem in the leapp-repository PR. I will continue in deeper investigation tomorrow with fresh mind, but I am considering it actually already good enough.

Before the merge:

  • polish commits (squash lint fixes, etc.)
  • fix the docstring "misinformation" noted by @matejmatuska

abadger and others added 3 commits November 12, 2024 21:57
@abadger
configs: implement actor configuration support
201493f 
This commit introduces multiple improvements and fixes for actor
configuration management in LEAPP, including configuration schema
support, API updates, validation improvements, and compatibility fixes.

- Actor Configuration:
  - Add actor configuration support
  - Introduce configuration schema attributes for LEAPP actors.
  - Create an API to load and validate actor configs against the schemas
    stored in actors.
  - Provide a function to retrieve actor-specified configurations.
  - Enable config directories at both repository-wide and actor-wide
    levels.
  - Add configs to `leapp.db`.

- Configuration Schema Support:
  - Add `_is_config_sequence()` to validate sequences (lists, tuples) of
    configuration fields.
  - Add support for `StringMap` field types in `config_schemas`.

- Testing and Linting:
  - Separate linting and unittests for better CI control.

- Dependency Management:
  - Add `pyyaml` to requirements in `requirements.txt`, `setup.py`, and
    spec files.

Some unit tests were also updated as they rely on `os.chdir` which
is problematic as the patch tries to correctly reverse any os.chdir
that is executed. Therefore, we mock `os.chdir` in the appropriate
unit tests.

JIRA: OAMG-8803
pylint: ignore too many params/lines, yield from
9cb0a74 
Ignore pylint's options:
* too-many-lines
* too-many-positional-arguments
* use-yield-from

Some of these options cannot be used (python2.7 compat) or configured.
See: oamg/leapp-repository#1299
packaging: bump leapp-framework version
7efbd20 
Increase the provided leapp-framework version to 6.0 (from 5.0).
Similarly, framework_dependencies are bumped to 6.0. These changes
are done as introducing actor configuration presents a major feature,
and, therefore, any codebases using the framework might want to
require leapp with config support. Bumping the version number, thus,
allows to distinguish between leapp-framework versions that provide
configs and those that do not.
@pirat89
Copy link
Member

pirat89 commented Nov 13, 2024

ignore failed tests on RHEL 7.9. there are some hicupps due to yum and some special requirement to have a build from leapp-repository PR. it's ok

pirat89
pirat89 approved these changes Nov 13, 2024
View reviewed changes
Copy link
Member

@pirat89 pirat89 left a comment

Choose a reason for hiding this comment

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

It's good to go!!!

@pirat89
Copy link
Member

pirat89 commented Nov 13, 2024

manually tested on AWS for IPU 7 -> 8 and 8 -> 9 as well. work as expected.

@pirat89 pirat89 merged commit 607be4c into oamg:main Nov 13, 2024
24 of 28 checks passed
@pirat89 pirat89 added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejmatuska matejmatuska matejmatuska left review comments

@dkubek dkubek dkubek left review comments

@vinzenz vinzenz vinzenz left review comments

@MichalHe MichalHe MichalHe approved these changes

@pirat89 pirat89 pirat89 approved these changes

Assignees

@pirat89 pirat89

@MichalHe MichalHe

@matejmatuska matejmatuska

Labels
changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant enhancement
Projects
None yet
Milestone
8.10/9.6
Development

Successfully merging this pull request may close these issues.
6 participants
@abadger @dkubek @MichalHe @pirat89 @vinzenz @matejmatuska