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.

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

Add coverage for BZ1964539 #14519

Merged
merged 2 commits into from
Apr 15, 2024
Merged

Add coverage for BZ1964539 #14519

merged 2 commits into from
Apr 15, 2024

Conversation

rmynar
Copy link
Contributor

@rmynar rmynar commented Mar 26, 2024

Adding a test case to cover BZ1964539
Satellite and any of it's dependencies should not require package nmap-ncat.
The test is parametrized, so it can be easily extended to test different packages.

@rmynar rmynar added QETestCoverage Issues and PRs relating to a Satellite bug CherryPick PR needs CherryPick to previous branches 6.15.z Introduced in or relating directly to Satellite 6.15 labels Mar 26, 2024
@rmynar rmynar requested a review from a team as a code owner March 26, 2024 10:02
@rmynar
Copy link
Contributor Author

rmynar commented Mar 26, 2024

trigger: test-robottelo
pytest: tests/foreman/installer/test_installer.py -k 'test_weak_dependency or test_content_guarded_distributions_option or test_capsule_installation or test_foreman_rails_cache_store'

@rmynar
Copy link
Contributor Author

rmynar commented Mar 26, 2024

trigger: test-robottelo
pytest: tests/foreman/installer/test_installer.py -k test_weak_dependency

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6203
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/installer/test_installer.py -k test_weak_dependency --external-logging
Test Result : =========== 1 passed, 23 deselected, 1 warning in 2109.00s (0:35:08) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Mar 26, 2024
Copy link
Contributor

@Griffin-Sullivan Griffin-Sullivan left a comment

Choose a reason for hiding this comment

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

Seems like the test will cover what was in the BZ. My main concern is if this test is really necessary since we are only testing the one packages. I'd love to find a way to make this have a bigger impact with more than just one package. @lpramuk @jameerpathan111 @ehelms @evgeni @ekohl any thoughts around this?

tests/foreman/installer/test_installer.py Outdated Show resolved Hide resolved
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Mar 26, 2024
@ekohl
Copy link
Contributor

ekohl commented Mar 26, 2024

I wonder what the best way of approaching this is. I'd imagine you can test this out without installing things which is a lot faster. How about this:

  • Get a plain RHEL
  • Assert nmap is not installed
  • Set up repositories
  • assert 'nmap' not in run('dnf --setopt=install_weak_deps=False --assumeno install satellite')
  • assert 'nmap' not in run('dnf --setopt=install_weak_deps=True --assumeno install satellite')

You may need to tune the package list and you can probably parametrize it, or even skip the False case but it wouldn't save a lot of time in this case while it may make debugging easier.

Another approach would be to work with delivery to get this in a regular pipeline. They will investigate the failures of those and prevent it from even promoting in the first place.

@lhellebr
Copy link
Contributor

Why test using dnf remove?

Also, Ewould has a point, it may be better to check for package presence somewhere else. My suggestion would be logs, the same way installer log is checked. Additionally, at the time when the logs are checked (whatever it is), you could maybe directly check for rpm presence.

@rmynar
Copy link
Contributor Author

rmynar commented Mar 27, 2024

I would definitely like to make this bigger, that's why it is parametrized.

Current solution uses the fixture sat_non_default_install which is common for more testcases in this module. So there's no significant additional time cost.

As said in the BZ, the unwanted dependency does not usually come from the main package itself (satellite.*rpm), but from some other subcomponent/feature which is not installed by default. This is the reason why I chose the opposite approach, i.e. enable various non-default subcomponents/features, attempt to remove unwanted package(s) and check if it does not remove any of our packages. As a benefit we also test if the non-default subcomponents/features can be installed during the fixture setup.

@ekohl
Copy link
Contributor

ekohl commented Mar 27, 2024

As said in the BZ, the unwanted dependency does not usually come from the main package itself (satellite.*rpm), but from some other subcomponent/feature which is not installed by default. This is the reason why I chose the opposite approach, i.e. enable various non-default subcomponents/features, attempt to remove unwanted package(s) and check if it does not remove any of our packages. As a benefit we also test if the non-default subcomponents/features can be installed during the fixture setup.

I tried to find a way to install packages in bulk, but dnf install foreman-plugin-\* to install all plugins doesn't work. What does work is using repoquery. In upstream:

# dnf repoquery --whatprovides 'foreman-plugin-*'
Last metadata expiration check: 0:01:23 ago on Wed Mar 27 20:27:43 2024.
rubygem-foreman-tasks-0:9.1.1-1.fm3_11.el9.noarch
rubygem-foreman_acd-0:0.9.4-3.fm3_10.el9.noarch
rubygem-foreman_ansible-0:14.0.0-1.fm3_11.el9.noarch
rubygem-foreman_azure_rm-0:2.2.11-2.fm3_11.el9.noarch
rubygem-foreman_bootdisk-0:21.2.2-1.fm3_11.el9.noarch
rubygem-foreman_concrete-0:1.0.0-4.fm3_10.el9.noarch
rubygem-foreman_datacenter-0:2.3.0-6.fm3_10.el9.noarch
rubygem-foreman_default_hostgroup-0:7.0.0-1.fm3_10.el9.noarch
rubygem-foreman_dhcp_browser-0:0.0.8-6.fm3_10.el9.noarch
rubygem-foreman_discovery-0:24.0.1-1.fm3_10.el9.noarch
rubygem-foreman_dlm-0:2.0.0-3.fm3_10.el9.noarch
rubygem-foreman_expire_hosts-0:8.1.0-1.fm3_10.el9.noarch
rubygem-foreman_fog_proxmox-0:0.15.0-1.fm3_10.el9.noarch
rubygem-foreman_git_templates-0:1.0.6-3.fm3_10.el9.noarch
rubygem-foreman_google-0:2.0.0-2.fm3_10.el9.noarch
rubygem-foreman_graphite-0:0.0.3-9.fm3_10.el9.noarch
rubygem-foreman_hdm-0:1.0.0-1.fm3_11.el9.noarch
rubygem-foreman_hooks-0:0.3.17-3.fm3_10.el9.noarch
rubygem-foreman_host_extra_validator-0:0.2.2-1.fm3_10.el9.noarch
rubygem-foreman_kubevirt-0:0.2.0-1.fm3_11.el9.noarch
rubygem-foreman_leapp-0:1.2.0-1.fm3_10.el9.noarch
rubygem-foreman_monitoring-0:3.0.0-1.fm3_10.el9.noarch
rubygem-foreman_netbox-0:1.2.0-1.fm3_10.el9.noarch
rubygem-foreman_omaha-0:5.0.1-2.fm3_10.el9.noarch
rubygem-foreman_openscap-0:7.1.1-2.fm3_10.el9.noarch
rubygem-foreman_probing-0:0.0.4-2.fm3_10.el9.noarch
rubygem-foreman_puppet-0:6.2.0-1.fm3_10.el9.noarch
rubygem-foreman_remote_execution-0:13.0.0-1.fm3_11.el9.noarch
rubygem-foreman_rescue-0:3.0.0-3.fm3_10.el9.noarch
rubygem-foreman_salt-0:16.0.1-1.fm3_11.el9.noarch
rubygem-foreman_setup-0:8.0.1-2.fm3_10.el9.noarch
rubygem-foreman_snapshot_management-0:3.0.0-2.fm3_11.el9.noarch
rubygem-foreman_statistics-0:2.1.0-2.fm3_10.el9.noarch
rubygem-foreman_supervisory_authority-0:0.0.2-4.fm3_10.el9.noarch
rubygem-foreman_templates-0:9.4.0-2.fm3_10.el9.noarch
rubygem-foreman_vault-0:1.2.0-1.fm3_10.el9.noarch
rubygem-foreman_vmwareannotations-0:0.0.1-6.fm3_10.el9.noarch
rubygem-foreman_webhooks-0:3.2.2-2.fm3_10.el9.noarch
rubygem-foreman_wreckingball-0:4.0.0-3.fm3_10.el9.noarch
rubygem-puppetdb_foreman-0:6.0.2-1.fm3_10.el9.noarch

So I'd this is a way to install satellite and all optional plugins: dnf install satellite $(dnf repoquery --whatprovides 'foreman-plugin-*' -q)

We don't have an equivalent provides for Hammer or Smart Proxy plugins, so you're stuck with globbing the package name with rubygem-hammer_cli_*.

Note that none of this needs actual installation of packages so you can really use a bare RHEL with the right repos set up.

@lpramuk
Copy link
Contributor

lpramuk commented Apr 8, 2024

Currently nmap is a weak dependency of foreman-discovery-image.
The test is going to ensure that it is going to stay that way:
customers are able to uninstall nmap if they care about nmap not present

@rmynar
Copy link
Contributor Author

rmynar commented Apr 9, 2024

I slightly updated test docstring to make it more clear that current solution tries to be as close as possible to customer experience. As reported in the BZ, the customer had an existing instance of Satellite and he could not remove potentially harmful package because it would also remove some satellite or foreman package(s).
I added pit marker so we're able to catch dependency error earlier.

@lpramuk
Copy link
Contributor

lpramuk commented Apr 15, 2024

I wonder what the best way of approaching this is. I'd imagine you can test this out without installing things which is a lot faster. How about this:

* Get a plain RHEL

* Assert `nmap` is not installed

* Set up repositories

* `assert 'nmap' not in run('dnf --setopt=install_weak_deps=False --assumeno install satellite')`

* `assert 'nmap' not in run('dnf --setopt=install_weak_deps=True --assumeno install satellite')`

You may need to tune the package list and you can probably parametrize it, or even skip the False case but it wouldn't save a lot of time in this case while it may make debugging easier.

Another approach would be to work with delivery to get this in a regular pipeline. They will investigate the failures of those and prevent it from even promoting in the first place.

This PR covers the BZ
if you are interested in generic packaging testing then this is definitely out of scope of this PR

Copy link
Member

@ogajduse ogajduse left a comment

Choose a reason for hiding this comment

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

This test fully covers the related BZ. Radek chose a different approach from what @ekohl suggested, but it still covers the product bug. The test will pass also in a situation when this dependency is not present in the system or if it got renamed. That is a downside of the current form of the test.
LGTM otherwise. I see no reason to block it at the moment.

@ogajduse ogajduse enabled auto-merge (squash) April 15, 2024 13:29
@ogajduse ogajduse merged commit db5b3d5 into SatelliteQE:master Apr 15, 2024
8 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 15, 2024
(cherry picked from commit db5b3d5)
pondrejk pushed a commit that referenced this pull request Apr 17, 2024
Add coverage for BZ1964539 (#14519)

(cherry picked from commit db5b3d5)

Co-authored-by: rmynar <[email protected]>
jyejare pushed a commit to jyejare/robottelo that referenced this pull request Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.15.z Introduced in or relating directly to Satellite 6.15 CherryPick PR needs CherryPick to previous branches QETestCoverage Issues and PRs relating to a Satellite bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants