oamg / leapp-repository

Leapp repositories containing actors for the Leapp framework (https://github.com/oamg/leapp). Currently provides leapp repositories for in-place upgrades of RHEL systems.
Apache License 2.0
49 stars 144 forks source link

No easy way to skip some actors. #823

Open holser opened 2 years ago

holser commented 2 years ago

Actual behavior There is no way to easily disable or enable actor. The only way to disable actor is to delete it. However, recent refactoring of leapp broke OpenStack CI as path of actor was changed from

/usr/share/leapp-repository/repositories/system_upgrade/el7toel8/actors/ to /usr/share/leapp-repository/repositories/system_upgrade/common/actors/

I propose to introduce --skip-actors actor1,actor2 or introduce environment variable to be able to disable some actors for CI purposes. That will allow to use cli option no matter how leapp is organized or refactored underneath.

To Reproduce Install leapp-0.12 and delete actor Install leapp-0.13 and delete actor using same script.

Expected behavior Consistent interface for disabling actors.

pirat89 commented 2 years ago

@holser The proposal does not make sense from the design POV. With the option you would be able to skip particular actor, which could lead in various breaks / issues / crashes / unexpected outputs ... basically it is breaking the design and provides more problems than solutions. In the same time, it does not solve your problem, as the name of the actor can be changed as well - which happened several times already, even during the last year IIRC.

First, we are missing examples why some actors needs to be removed.

Can you provide reasons why to skip some actors that are not included above?

btw, the suggested feature is supposed to be reported in leapp-repository, as this is the thing of particular repositories (including leapp CLI commands included in them). I will transfer it.

pirat89 commented 2 years ago

Discussed the possibility to provide LEAPP_DEVEL_SKIP_ACTORS=... possibility instead, that probably makes more sense, however, still there is the question about reasons for such functionality.

holser commented 2 years ago

The reason is quite simple. The leapp is used in Ecosystem products such as OpenStack or OpenShift to test upgrades. When actor behavior was changed it may act differently which may block CI for a quite a long time. Instead of waiting on new release of leapp a simple workaround may be provided instead.

holser commented 2 years ago

Another reason is CI. If you have changes in single actor you may disable all other actors to test only particular one.

vinzenz commented 2 years ago

On Wed, Feb 2, 2022, 16:57 Sergii Golovatiuk @.***> wrote:

Another reason is CI. If you have changes in single actor you may disable all other actors to test only particular one.

Well for CI we have make test ACTOR=(dirname|actor classname|name)

Reply to this email directly, view it on GitHub https://github.com/oamg/leapp-repository/issues/823#issuecomment-1028087739, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAI6HVFGDMW76WHMVPMGKTUZFH57ANCNFSM5M25L5WQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

odyssey4me commented 2 years ago

Here are some examples of actors we have found ourselves removing in some engagements.

persistentnetnamesdisable:

biosdevname:

checkinstalledkernels:

These are decisions made with extensive testing to validate whether they have negative side-effects or not. The point is that it is something that ends up being done, so it'd be better to create an interface to do it which allows the code to be moved around freely while layered products, consulting, documentation, etc can all remain constant.

holser commented 2 years ago

Also I wanted to mention

checkkerneldrivers:

vinzenz commented 2 years ago

Well, we will keep this in mind, however how things are going right now, there is no way something like this is going to happen in the next release. We will see what we can come up with.

@odyssey4me

... so it'd be better to create an interface to do it which allows the code to be moved...

The problem is that you are asking for an interface where by design was never meant to be an 'interface' and as soon as we would add an interface, we would have to call this 'supported' which we inherently do not want.

There are reason for those actors to be present, and yes there is the possibility that YMMV. If you delete them as a workaround this is on you then to ensure it goes well. Not saying that there aren't any bugs, we can work on those of course and as an interim solution you could delete the actors doing the checks so your CI systems keep working. But it's not a solution for everyone.

Anyway we'll have to have a discussion about this and what we can do to make it maybe 'easier' for you. But it I am pretty sure that the upgrade always will be considered as unsupported.

holser commented 2 years ago

@vinzenz

There are a lot of different interfaces with different purposes. It's all in our hands how we design and target the interface. As discussed with @pirat89 on IRC we may create LEAPP_DEVEL_SKIP_ACTORS= as interfaces. DEVEL in its name should help end users understand that interface is designed for developers and their needs. Additionally we may specify that in documentation as well.

pirat89 commented 2 years ago

@holser but keep in mind that this rfe has super low priority for us and still it has not been decided it will be implemented really. Most of mentioned problems are not good enough reasons for the skipping of actors.