PR#85: Dont update req packages to latest - standard-test-roles

standard-test-roles

#85 Dont update req packages to latest

Merged 6 years ago by astepano. Opened 6 years ago by jbieren.

jbieren/standard-test-roles master into master

Dont update req packages to latest

Johnny Bieren • 6 years ago

da7a687

roles/standard-test-beakerlib/tasks/main.yml

file modified

+1 -1

		`@@ -73,7 +73,7 @@`

		`- name: Install any test-specific package requirements`
		`# Note, this method cannot install -debuginfo packages.`
		`- package: name={{item}} state=latest`
		`+ package: name={{item}} state=present`
		`with_items:`
		`- "{{ pkgs_ordinary_req }}"`

jbieren commented 6 years ago

To the best of my knowledge, this task is installing the packages listed in tests.yml as required_packages. I think this can cause an issue with the package you are targeting, as the tests.yml file includes said package. For example, in
https://upstreamfirst.fedorainfracloud.org/systemd/blob/master/f/tests.yml
systemd is a required package. Thus, say the latest available systemd rpm is version 236, but you are testing a custom built rpm (maybe from a PR?) and your build system gave your rpm version 1. This task will now update from your test systemd-1 to systemd-236, and thus your testing is not useful. I agree latest makes sense for all required_packages that aren't the package under test, but I think because of tests.yml files including the package under test as a required_package, state=latest can't be used here.

Signed-off-by: Johnny Bieren jbieren@redhat.com

astepano commented 6 years ago

Yes, I agree. But what are reasons to:

List 'systemd' is in the list 'required_packages'. dist-git tests runs against builds/composes/packages/etc. Test subject already holds necessary package version. It is responsibility of CI Pipeline to provide necessary package in build/compose. Personally I do not see any reason to include systemd in the list 'required_packages'.
The default behavior of beakelib role: is to install the latest package. If we change this, it will influence all tests that we have now. It is very serious change. If you need to specify package version it is better to do this in other way. Ansible is quite flexible, you can write your own Ansible task for installing some package that doesn't fit to common scheme. Test interface is quite flexible. standard-test-roles is supplemental package. tests.yml can be modified as you wish in case of specific cases when standard-test-roles doesn't fit.

Therefore, I would refuse this PR.

jbieren commented 6 years ago

I totally agree with you. I also don't know what the reasoning behind including the package under test as a required_package is. I figured it would be easier to switch it here than it would be to open PRs against the however many tests.yml files of the (100?) packages that include it. The CI Pipeline will provide the necessary package in the build/compose. However, if we are ingesting a test subject rpm from another build system outside of the ci pipeline, we can't really make any guarantees that yum update foo isn't going to move us from the test version of foo to the latest in epel.
I don't follow you. Neither before or after this change is anything specifying a package version. I agree if you want to install a specific package, you can write your own task to do so. The only thing this PR changes is if a package is already present on the system under test, it leaves it as is instead of trying to update to latest. I'm not sure what the gain is of trying to update already present packages before running the testing. Imho, all that should matter is that they are there. If the test requires the testing interface to cleverly update a package to latest behind the scenes, I think that test should be changed to be more explicit and clear, in my eyes. And agreed, it is a supplemental package that can be used in other scenarios, but my understanding was that the primary purpose of standard-test-roles was to run the tests.yml files that were being ported to dist git. Thus, making a change that would fix anywhere from 1- ~100 tests.yml files would be worth it.