Introduce rpms.get_leapp_packages library function

[fernflower]

This function can be used as leapp-installtion-packages-getter. Refactoring of other actors using it will be done later.

[github-actions]

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

• review please @oamg/developers to notify leapp developers of the review request
• /packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp from PR#42, use /packit test oamg/leapp#42

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones.
To launch on-demand tests with packit:

• /packit test --labels kernel-rt to schedule kernel-rt tests set for all upgrade paths
• /packit test --labels beaker-minimal-8.9to9.3,kernel-rt-8.9to9.3 to schedule kernel-rt and beaker-minimal test sets for 8.9->9.3 upgrade path

[Deprecated] To launch on-demand regression testing public members of oamg organization can leave the following comment:

• /rerun to schedule basic regression tests using this pr build and latest upstream leapp build as artifacts
• /rerun 42 to schedule basic regression tests using this pr build and leapp*PR42* as artifacts
• /rerun-sst to schedule sst tests using this pr build and latest upstream leapp build as artifacts
• /rerun-sst 42 to schedule sst tests using this pr build and leapp*PR42* as artifacts

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra.

[abadger]

Second pass review. Some things here are minor and somethings won't apply if we change the api.

[pirat89]

tbh, for me it currently does not respect voting results (which we will discusse later; I am not saying the chosen form was the best one and possibly it would be better to go with the PR POC since the beginning, but I would appreciate to try it instead of ignoring it and go in own way).

the main problem I have with the current approach is that:

• it is incomplete - does not cover cockpit-leapp as we discussed
• it's adding snactor separately, which does not seem to me conceptual. how we will deal with leapp-inspector in future? the current logic suggest to add "inspector" value, which seems almost to me like we are already listing each one rpm by one value. this is getting more far from having it simple.

Also I prefer to have defined a "LeappComponent" class (just a work name) which defines particular values. So using the function, people will be required to put there one of defined values instead of strings. e.g.

get_leapp_packages(component=[LeappComponent.Repository])

[fernflower]

@pirat89 Sorry for taking the initiative, but I stand with what I said on cabal - pure implementation details should never be a matter of internal voting process in an opensource project as that's another layer of non-transparent unnecessary complexity we just don't want.
You are totally right about a dedicated enum, I will definitely do that (along with addressing @abadger implementation-related comments which I totally agree with).

[abadger]

pure implementation details should never be a matter of internal voting process in an opensource project

I agree with this sentiment but I wanted to think for a while about why I believe that before replying. I think in open source projects, we should be striving for consensus decision making whenever possible. That means that people will raise problems with a proposal (possibly giving a potential solution) and then the proposal should be modified to address the problem (sometimes with the potential solution and sometimes with something different). We reach consensus when all the problems have addressed.

Sometimes we will disagree on whether something is actually a problem or whether other constraints (time, unable to see another way that has less drawbacks due to other factors, etc) make the solution worse than the problem raised. For those types of disagreements, voting becomes a way to resolve the impasse.

Consensus decision making does require a few behaviours to work well:

• It requires moving away from expressing mere opinions about code (I think we should have two function to do this task) to being able to enunciate problems that our proposal would address (One parameter is asking to select more data and another parameter asking to filter some of them out. It's easier to read code that places addition and subtraction into separate functions) so that we all understand what the base problems someone is pointing at are.

• We have to be flexible in allowing other ways of doing something to occur as long as it addresses the base problems (What if instead of filtering, we make the caller select that data as well?)

• It's good to be proactive about pointing out problems in our proposals that we think others might want to propose alternatives to: (I made this a constant holding a frozenset of strings but I know some people like to use Enums more than me. I think it's okay to do this because it's not user visible but does someone have a different evaluation?)

Hope this is helpful.

[pirat89]

pure implementation details should never be a matter of internal voting process in an opensource project

I agree with this sentiment but I wanted to think for a while about why I believe that before replying. I think in open source projects, we should be striving for consensus decision making whenever possible. That means that people will raise problems with a proposal (possibly giving a potential solution) and then the proposal should be modified to address the problem (sometimes with the potential solution and sometimes with something different). We reach consensus when all the problems have addressed.

Sometimes we will disagree on whether something is actually a problem or whether other constraints (time, unable to see another way that has less drawbacks due to other factors, etc) make the solution worse than the problem raised. For those types of disagreements, voting becomes a way to resolve the impasse.

Consensus decision making does require a few behaviours to work well:

• It requires moving away from expressing mere opinions about code (I think we should have two function to do this task) to being able to enunciate problems that our proposal would address (One parameter is asking to select more data and another parameter asking to filter some of them out. It's easier to read code that places addition and subtraction into separate functions) so that we all understand what the base problems someone is pointing at are.
• We have to be flexible in allowing other ways of doing something to occur as long as it addresses the base problems (What if instead of filtering, we make the caller select that data as well?)
• It's good to be proactive about pointing out problems in our proposals that we think others might want to propose alternatives to: (I made this a constant holding a frozenset of strings but I know some people like to use Enums more than me. I think it's okay to do this because it's not user visible but does someone have a different evaluation?)

Hope this is helpful.

I basically agree on that. To be clear, I do not have problem to do things differently in future. The only problem I've seen here was that we had 2 different places for discussions & decisions in this case, where some people discussed & voted in the first one (the one we started with), and another part of the team started different approach here. Which is inconsistent and does not meet the original expectation. Again, I am admitting the original way I chose (inspired by the usual one we used over years) in this case from the start was not the best one and this (PR) would be probably better approach. But it started in that way and it set already some expectations. I am in favor to do things differently in future (added topic on retro to discuss it), but I am urging to stay consistent, so people in team know what to expect and they are not surprised & confused later, that reality is different from what has been told - and also, we have more people involved in the process, the split resulted actually in situation when we haven't achieved the main reason why I wanted some voting around -- to involve as many people in the team as possible. I am keeping the discussion on the retro. What happened, it happened. I do not want to make feel anyone bad about that as everyone is making mistakes (including me, especially in this case). Instead, let's finish it and discuss how we will approach such situations (e.g. when voting is wanted) in the future.

[pirat89]

I finished my review (sorry I did it on several times, but I didn't want to work during my PTO so I finished the whole pre-final review now. I am still unsure about some stuff but it needs to be discussed via vcall as I do not see how we could progress there soon enough as we have time pressure here. So hopefully we will have discussion about that at least in a group of people during our canban mtg.

The fingerprint of the function voted originally is following:

leapp.libraries.common.rpms.get_leapp_packages(
    major_release: str|list,
    project: list [framework, repository, cockpit-leapp],
    include_deps: bool
)

In case of the project name, I am ok to go with component instead. The rest should be still discussed.

[pirat89]

I would like to know your opinion(s) on that guys. I am thinking in this way:

• we are responsible that the src_version is defined same for all Leapp.Components.
• If some new component is added, or some existing dropped, it should return empty list.
• If I create a generic actor, that will use this function for all upgrade paths, and we realize it will crash under specific conditions for a new major upgrade path, it could be still too late (even unit-tests will not count with that as they will be written based on the situation that exists during writting the code calling this function).

The implementation does not need to be changed, but in case we have same understanding / opinion on the scenariou I mentioned above, it would make sense to put this check before the loop, to check just the generic list of possible values. In this case ['7', '8'] that will be soon extended by '9'. WDYT? @oamg/developers

To be clear, this one is not blocker, but it's better to point this out so people think about that.

[fernflower]

Sorry, but I am against overcomplicating the logic at this point and committing ourselves to the Leapp.Components support cycle. The goal of this pr is to introduce a library helper - nothing more and nothing less.

[pirat89]

once this is part of the library, it's already supported by us.

[fernflower]

Sorry, I am completely missing what you are suggesting here, I see only many ifs and fear of something failing that is not even there yet.

If something crashes we fix it. The api doesn't change either way. What are we discussing here?
If you see a flaw in the implementation - please provide a clear minimal case when it fails, I am happy to cover it with a unit test and deliver a fix.

[pirat89]

Sorry, I tried to provide the example in the description of this thread. But I will try it in a different way. So e.g.:

_MY_LEAPP_PACKAGES_MAP = {
    "framework": {'7': {'pkgs': ['leapp', 'python2-leapp'],
                                          'deps': ['leapp-deps']},
                                    '8': {'pkgs': ['leapp', 'python3-leapp'],
                                          'deps': ['leapp-deps']}
                                    },
    "newcomponent":  {'8': {'pkgs': ['leapp', 'python3-leapp'],
                                          'deps': ['leapp-deps']}
                                    },

calling get_leapp_packages("7", "newcomponent") ends with an error now. As mentioned in the start of this thread, I will point out one more time key things:

• I think we should in such a case return empty list
• valid values in such a scenario are "7", "8".
• such a scenario should not happen (I mean that we should define all, right?), and when the map is changed in any way, all releases should be always defined. so in case of "newcomponent" should be always defined "7" & "8" in this scenario
  • it's simple to cover such a scenario in the code, where having undefined release lead to an empty list

Why I started the thread is, that if we agree on that, we can modify the implementation a little bit, to make it more clear and safe. Which would mean putting one if statement out of the loop, and in a case we would like to keep some releases "undefined", we could add one if into the loop to automatically act like an empty list is defined. pseudocode:

def get_leapp_packages(...):
    ...
    if major_release and major_release not in ["7", "8", ..]:
       raise ValueError
   ...
   for comp in components:
       if release not in _MAP[comp]:
           # nothing to do - release is valid, but undefined for the particular component
           log.warning(...) 
           continue

Yes, there is not such a problem now. It is not clear whether it happens (it's possible we will drop the cockpit-leapp for rhel 10). Having a generic actor that work for all upgrade paths, I can imagine it would be better to not introduce tracebacks in such a case, which would need to be trated by additional "ifs" in actors. to discover, what values are possible for what components, as an actor developer, I would have to otherwise open the code, and check all possible values per each component I can use.

Note that in the described scenario above, the traceback would be originally raised also by get_leapp_packages(major_release="7"). Not specifying all components, expecting to get all packages valid for one particular major release. the change in the implementation would prevent such situations, in case of the incomplete map.

@oamg/developers hopefully this explanation is more clear

[fernflower]

Why should the behavior be different for get_leapp_component('7', 'nosuchcomponent') from raising ValueError as we agreed before? IIRC we had this changed specifically because we wanted to provide clear fast failure for anyone who uses it wrong, and the empty list doesn't help much here.
Imho having 2 types of behavior is much worse than returning empty list or raising exception on all occasions.

[pirat89]

@fernflower because for me it's different whether I put call get_leapp_component("1") - with completely nonsense value for a release that we do not cover anywhere in the code / that is unsupported at all, and sitaution when get_leapp_component("7")` is valid but our mistake that the map is not full. As I wrote, we can take full responsibility, so with any change, the author and reviewer will ensure the map will be always complete. But I raise that to think about that, that we could prevent us from issues. As I wrote, it's not blocker, I just started the public discussion to get people' opinions

[fernflower]

Okay, my position stands - I have no strong feelings about ValueError (crash early in case of wrong usage) or empty list (not getting a value and possibly messing up reports), but I am strictly against a mix of both, as it has the worst of both worlds unfortunately.

[pirat89]

Thanks for your opinion. As I mentioned, this is not blocker and it can be changed in future in case we will want as now there is not yet an existing scenario in the code for the behaviour we are discussing. Fix please the other stuff as we discsused so we could finish this PR. Keeping this thread opened for others.

[abadger]

I think we should raise ValueError in case there is an unknown component, but your point about making it easier for people to write generic actors is valid. I think we could solve it in another way. However, I can also see two scenarios: One in favour of making it easier and one in favour of forcing the caller to update their code:

In favour of making it easier:

• leapp-cockpit needs to be factored into the RHEL 7 and 8 upgrade.
• leapp-cockpit must not be factored into the RHEL 9 upgrade (perhaps the package is not shipped on that distro version).
• Calling code had: get_leapp_packages(components=(LeappComponents.FRAMEWORK, LeappComponents.COCKPIT))

In this case, if we did not error due to the use of LeappComponents.COCKPIT, there would be no problem and the caller would appreciate not having to modify their code to handle both 7&8 upgrades and 9 upgrades.

In favour of forcing an update:

• leapp-cockpit for the RHEL 9 upgrade, leapp-cockpit and snactor must be installed together. Therefore, leapp-cockpit is moved into the TOOLS component.
• Calling code had: get_leapp_packages(components=(LeappComponents.FRAMEWORK, LeappComponents.COCKPIT))

In this case, we need to force the caller to update their code. If they do not get an error, they will not get both leapp-cockpit and snactor returned as leapp_packages which will be a bug in their code.

Solution that handles both (but may be overly complex):

• For the first use case, LEAPP_DEFAULT_COMPONENTS can continue to have an entry for COCKPIT but it will be an empty list. Then, there will be no error in the calling code.
• For the second use case, COCKPIT must be removed. That way callers will be forced to update their code to do the right thing.

The danger in having two different ways of handling this as just an entry or lack of entry in the mapping is that someone might not realize that a particular change should follow one or the other strategy and introduce a bug by doing the wrong thing (this could even be a regression if someone thought they were fixing a bug in another PR but in reality, it was doing the right thing by raising an exception/not raising an exception initially).

How to make sure that testing catches errors?

We can write a unittest that will catch whether the code will throw errors or not throw errors when we don't expect it by looping through the components listed in the mapping and making sure that ones we do not want to throw errors exist in all releases. However, as noted in the previous section, this test is only as good as we are in making sure the test is correct. If the test failure doesn't explain the issue well enough to the developer, they will just change the test instead of realizing their code change is a mistake.

[pirat89]

@oamg/developers adding comments from the discussion we have with @fernflower and @Rezney:

• we agreed to stay with the current set of components (framework, reposutory, cockpit, tools)
  • docstring will explain what to expect from each of them
• we have decided to with the other earlier discussed proposal, to get deps packages from a different function. in this case another function (e.g. get_leapp_deps_packages or sth like that) will be created, that will have major_version and component parameters.

So include_deps parameter is going to be dropped.

Correct me if I missed something.

[fernflower]

Resolved all comments but splitting the function into 2 (one returns deps packages, one doesn't). Having slept on this I'd like to gather some more input from other folks, especially @matejmatuska who needs that and @dkubek .

[abadger]

Looks good. I have a set of suggestions that would make it easier for callers to make simple modifications to the default components which are requested. What do you think of those?

[abadger]

SUGGESTION_TAG: easier_component_modification

Suggested change

	(use enum :class: LeappComponents) If defined then only packages related to the specific
	component(s) will be returned.
	(use enum :class: LeappComponents) If defined then only packages related to the specific
	component(s) will be returned. The default set of components is in
	`LEAPP_DEFAULT_COMPONENTS` and simple modifications of the default can be achieved
	with code like:
	.. code-block:: python
	get_leapp_packages(
	component=LEAPP_DEFAULT_COMPONENTS.difference(
	[LeappComponents.TOOLS]
	))

[abadger]

This (and the code to go along with it) has been added as e61b212 EDIT There were some force pushes so the new commit hash is: 4c73994

@pirat89 @fernflower If you like the change in that commit, we can squash everything together. If you don't like it, we can revert just that one commit and discuss it in a different PR.

All requested changes fixed except for making two functions (which will be discussed further).

[fernflower]

@abadger thank you, looks perfect to me.
@pirat89 review please

stale

[pirat89]

Looks great to me. GET_LEAPP_PACKAGES_DEFAULT_COMPONENTS is a little bit longer, but I believe it's not a problem. The new name is clear for me. Merging.