remove sat-version-check hook from HostCollection class

[rplevka]

Since we now use different branches for different versions, checking for satellite version in order to take appropriate action makes no sense anymore. Our automation does not even use the version correctly and related tests fail on 6.2 pipeline.

https://github.com/SatelliteQE/nailgun/blob/272ff30ea0d99f15a4c326d2891536f70d2072d4/nailgun/entities.py#L2140

[omaciel]

@rplevka are you suggesting that we remove the conditional statements and code that don't apply to the version of Satellite being tested?

[rplevka]

@omaciel, correct. Originally I wanted us to put appropriate code into 6.1.z and 6.2.z branches, however i noticed there's no 6.1.z branch (why?). Instead, we pinned the release of nailgun to 0.25.0 for robottelo/6.1.z (so we actually don't use any of the changes introduced after). I believe it is safe to remove the 6.1-related condition and move on with 6.2+ changes

I'd like @SatelliteQE/quality-engineers who have the insight to chime in as I might be terribly wrong ;)

thank you

[rplevka]

btw, the reason, why I opened this is because I found out this actually hasn't ever worked correctly in 6.2.z automation and causes test failures (mainly in tier4).

tests.foreman.longrun.test_inc_updates > IncrementalUpdateTestCase::test_positive_noapply_cli

E           NoSuchFieldError: Valid fields are ['description', 'max_hosts', 'unlimited_hosts', 'host', 'organization', 'id', 'name'], but received ['max_content_hosts', 'name', 'organization'] instead.

[sthirugn]

If the version check never worked, I am wondering how the other tests which depend on satellite version worked in nailgun for 6.2.

[elyezer]

If the version check never worked, I am wondering how the other tests which depend on satellite version worked in nailgun for 6.2.

NailGun always consider the latest version, for example if something is new then a if block is added for the past version and the new behavior is the default one.

Also it has the function _get_version which ensures running the latest behavior https://github.com/SatelliteQE/nailgun/blob/master/nailgun/entities.py#L175-L193

[elyezer]

btw, the reason, why I opened this is because I found out this actually hasn't ever worked correctly in 6.2.z automation and causes test failures (mainly in tier4).

It never worked because Robottelo does not configure the version, that is not NailGun fault but Robottelo's, see https://github.com/SatelliteQE/robottelo/blob/master/robottelo/config/base.py#L914-L918. It should pass the version under testing and it was never done because robottelo does not have that information.

[rplevka]

@elyezer then the version-oriented hooks should be removed from nailgun at least from the 6.2.z branch. I admit the hooks made sense until nailgun has been branched. Now 6.2.z branch is supposed and meant to be run on only on 6.2.* versions of satellite, thus any further version checking lacks sense.

[elyezer]

And how would we align this with the version of Pulp Smash? Remember Pulp Smash is a separated product which was designed to be able to communicate with different versions of Satellite using the entities.

Removing that we are saying now that we will support just an specific version of satellite on the next release of Satellite. Also if something needs to be updated onto the 6.2.z branch how would that be released?

I am not leaving my opinion yet but bringing the points from the project perspective so we can make an agreement. I would like to know what others think about it.

[sthirugn]

I agree with @rplevka here and this is what I have been advocating for a long time now. We should just follow the dev model here:

• Most of the time, we do not spend time fixing automation code for an older Satellite version and focus only on the master - The issue which @rplevka just discovered is just an example of this :)
• It is okay to support only the latest Satellite version in nailgun master. If we really need to backport something very important to an older version, we need to cherrypick, which I don't see us doing much anyways. There is no point in continuing to write ugly code like this (see below) anymore. Simplicity is the way to go - together with proper branching as explained in the document linked below.

if version == 6.1 `do only this`
  else if version == 6.2 or 6.2.4 `do just this`
  else `error out`

Also:

• if pulp smash likes to communicate to older satellite version, it can use a specific branch to do so. This should not prevent us from discontinuing to maintain nailgun master for all different versions - it is just too much overhead and the overhead will continue to increase in the future.
• More information about branching process here: https://url.corp.redhat.com/b60264b

[elyezer]

Alright, so if we decide to use the branching model and support just the latest version of Satellite, which is totally fine, we should also consider how the version and releases of NailGun will be handled. We have some other tools like the Ansible module written by @ehelms and other tools that may be using NailGun and dropping that support by requiring the users to clone an specific branch and not install from PyPI seems not good to me.

I know that Robottelo is the great customer we have for NailGun but I am making sure that every decision made in the past are not just dropped because it will be easy to maintain without even thinking about them.

In my opinion dropping the versioning checking and bringing that to the git logic (using branches) have the same effort than dealing with if/else blocks. I am saying that because basically instead of having a new if block we will have another branch and that may require cherry picks which can make that also not easy to maintain.

So all this is just a matter of making a design decision, my vote is still for having NailGun to support many Satellite versions within the same installation (yes this may drop the branching model).

[renzon]

Hey guys, adding my 2 cents here:

Clearly there are two completely different use cases here:

1. pulp which need to talk with multiple versions;
2. robotello which only need to speak with one, accordingly to its branch (AFAIK)

For the second the intention would be pinning nailgun version accordingly to satellite. Or, to be more precise, with the API itself.

The problem is that would be impossible for pulp to work, once it is (probably) impossible working with different versions of same lib installed.

On the other hand keeping nailgun working for different versions make code complex. Check @Ichimonji10 commit when this approach took place. Now the class need to dynamically decide its fields taking into account API version.

Maybe a middle ground could be better for both use cases: we could have different explicity entities versions:

entities could be a package and inside it we could have: ent_v_6_3.py ent_v_6_2.py and so on

On init.py we could have only on line importing form the latest version:

from ent_v_6.3 import *

All the time API version changes (lets say v 6.4) we create a new file, change entitity.init.py

from ent_v_6.4 import *

With this, robotello could continue to import from entity and getting tha last version, based on nailgun version, so it would work for the its branches if we pin its version.

Pulp would import from specific versions files as need.

Nailgun could be installed from pypi

We could also make could simpler againg, returning with class attributes. There would be no "ifs" regarding API version on code and we could even compare version files to check how API evolved during time.

Drawbacks:

1. keeping all those files, having to port back fixes when needed
2. having to remember to update init while releasing a new entities compatible with different API version

My 2 cents ;)

[rochacbruno]

There is also the approach of having plugins

pip install nailgun

Then you can start customizing by installing extra plugins using the same namespace

pip install nailgun_62
pip install nailgun_63
pip install nailgun_64

That nailgun_ is a plugin which uses monkey patching to include or exclude fields, validations etc

PROS: We maintain a single base of nailgun having only the common stuff

CONS: We maintain different plugins (but small code), we decide which goes to main or to plugin based in tests, so we should have more test coverage on nailgun,

[rplevka]

@elyezer

In my opinion dropping the versioning checking and bringing that to the git logic (using branches) have the same effort than dealing with if/else blocks. I am saying that because basically instead of having a new if block we will have another branch and that may require cherry picks which can make that also not easy to maintain.

I don't 100% agree on this. There is a fundamental difference in the scenario of dropping support (EOL). While using branches would mean simply abandoning the outdated branch, 'if-else' hooks would be needed to be manually removed or still being maintained, keeping the code complexity

Now an idea regarding tagging and branching (possibly a naive one as i'm not of any expert in this field). Can't we use both? Why don't we stick to branching (robotello-like) per satellite Z stream and put release tags to the currently latest branch?

[elyezer]

We can schedule a meeting e talk about this, stay on this back and fort will not make anything happen.