Skip to content

Conversation

@ColeHiggins2
Copy link
Member

tests are failing in ipv6 because we do not have checks for network type.
Updating code to add a check for network type

    host['system_profile']['network_interfaces'][0]['ipv4_addresses'][0]
E   IndexError: list index out of range```

@ColeHiggins2 ColeHiggins2 self-assigned this Nov 20, 2025
@ColeHiggins2 ColeHiggins2 requested a review from a team as a code owner November 20, 2025 02:48
@ColeHiggins2 ColeHiggins2 added CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing Stream Introduced in or relating directly to Satellite Stream/Master 6.17.z 6.18.z Introduced in or relating directly to Satellite 6.18 labels Nov 20, 2025
@ColeHiggins2 ColeHiggins2 marked this pull request as draft November 20, 2025 02:48
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `tests/foreman/ui/test_rhcloud_inventory.py:168-166` </location>
<code_context>
+    if module_target_sat.network_type == NetworkType.IPV6:
+        # For IPv6 networks, check IPv6 addresses
+        ip_addresses = [
+            host['system_profile']['network_interfaces'][0]['ipv6_addresses'][0]
+            for host in json_data['hosts']
+        ]
+        ipv6_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
</code_context>

<issue_to_address>
**suggestion (testing):** Add error condition tests for missing or empty address lists.

Please include tests for cases where 'ipv6_addresses' and 'ipv4_addresses' are missing or empty to verify robust error handling.

Suggested implementation:

```python
    if module_target_sat.network_type == NetworkType.IPV6:
        # For IPv6 networks, check IPv6 addresses
        ip_addresses = []
        for host in json_data['hosts']:
            try:
                ipv6_list = host['system_profile']['network_interfaces'][0].get('ipv6_addresses', [])
                assert isinstance(ipv6_list, list), "ipv6_addresses should be a list"
                assert ipv6_list, "ipv6_addresses list is empty"
                ip_addresses.append(ipv6_list[0])
            except (KeyError, IndexError, AssertionError) as e:
                # Test error handling for missing or empty ipv6_addresses
                assert str(e) in [
                    "'ipv6_addresses'",
                    "ipv6_addresses list is empty",
                    "ipv6_addresses should be a list",
                    "list index out of range"
                ], f"Unexpected error: {e}"

        ipv6_addresses = []
        for host in json_data['hosts']:
            try:
                ip_list = host.get('ip_addresses', [])
                assert isinstance(ip_list, list), "ip_addresses should be a list"
                assert ip_list, "ip_addresses list is empty"
                ipv6_addresses.append(ip_list[0])
            except (KeyError, IndexError, AssertionError) as e:
                # Test error handling for missing or empty ip_addresses
                assert str(e) in [
                    "ip_addresses list is empty",
                    "ip_addresses should be a list",
                    "list index out of range"
                ], f"Unexpected error: {e}"

        if ip_addresses and ipv6_addresses:
            assert virtual_host.ip_addr in ip_addresses
            assert baremetal_host.ip_addr in ip_addresses
            assert virtual_host.ip_addr in ipv6_addresses
            assert baremetal_host.ip_addr in ipv6_addresses
    else:
        # For IPv4 networks, check IPv4 addresses
        ip_addresses = []
        for host in json_data['hosts']:
            try:
                ipv4_list = host['system_profile']['network_interfaces'][0].get('ipv4_addresses', [])
                assert isinstance(ipv4_list, list), "ipv4_addresses should be a list"
                assert ipv4_list, "ipv4_addresses list is empty"
                ip_addresses.append(ipv4_list[0])
            except (KeyError, IndexError, AssertionError) as e:
                # Test error handling for missing or empty ipv4_addresses
                assert str(e) in [
                    "'ipv4_addresses'",
                    "ipv4_addresses list is empty",
                    "ipv4_addresses should be a list",
                    "list index out of range"
                ], f"Unexpected error: {e}"

```

If you have separate test cases or fixtures for hosts with missing or empty address lists, ensure those are included in your test data to trigger these error conditions.
You may also want to refactor the error handling into helper functions if this pattern is repeated elsewhere in your test suite.
</issue_to_address>

### Comment 2
<location> `tests/foreman/api/test_rhcloud_inventory.py:94-95` </location>
<code_context>
-    assert baremetal_host.ip_addr in ip_addresses
-    assert virtual_host.ip_addr in ipv4_addresses
-    assert baremetal_host.ip_addr in ipv4_addresses
+    if module_target_sat.network_type == NetworkType.IPV6:
+        # For IPv6 networks, check IPv6 addresses
+        ip_addresses = [
+            host['system_profile']['network_interfaces'][0]['ipv6_addresses'][0]
</code_context>

<issue_to_address>
**suggestion (testing):** Missing negative tests for incorrect address types.

Please add tests to confirm that IPv4 addresses are not matched in IPv6 mode and vice versa, to prevent misclassification.

Suggested implementation:

```python
    if module_target_sat.network_type == NetworkType.IPV6:
        # For IPv6 networks, check IPv6 addresses
        ip_addresses = [
            host['system_profile']['network_interfaces'][0]['ipv6_addresses'][0]
            for host in json_data['hosts']
        ]
        ipv6_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
        assert virtual_host.ip_addr in ip_addresses
        assert baremetal_host.ip_addr in ip_addresses
        assert virtual_host.ip_addr in ipv6_addresses
        assert baremetal_host.ip_addr in ipv6_addresses

        # Negative tests: Ensure IPv4 addresses are NOT present in IPv6 lists
        assert virtual_host.ipv4_addr not in ip_addresses
        assert baremetal_host.ipv4_addr not in ip_addresses
        assert virtual_host.ipv4_addr not in ipv6_addresses
        assert baremetal_host.ipv4_addr not in ipv6_addresses
    else:
        # For IPv4 networks, check IPv4 addresses

```

You must ensure that `virtual_host.ipv4_addr` and `baremetal_host.ipv4_addr` are available and correctly set to the expected IPv4 addresses for the hosts. If these attributes do not exist, you should add them or otherwise obtain the correct IPv4 addresses for the negative tests.
Similarly, for the IPv4 branch, you should add negative tests to ensure that IPv6 addresses are not present in the IPv4 lists.
</issue_to_address>

### Comment 3
<location> `tests/foreman/api/test_rhcloud_inventory.py:94-115` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

### Comment 4
<location> `tests/foreman/ui/test_rhcloud_inventory.py:165-186` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

### Comment 5
<location> `tests/foreman/ui/test_rhcloud_inventory.py:279-300` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

### Comment 6
<location> `tests/foreman/ui/test_rhcloud_inventory.py:344-365` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

### Comment 7
<location> `tests/foreman/api/test_rhcloud_inventory.py:101` </location>
<code_context>
@pytest.mark.run_in_one_thread
@pytest.mark.e2e
def test_rhcloud_inventory_api_e2e(
    inventory_settings,
    rhcloud_manifest_org,
    rhcloud_registered_hosts,
    module_target_sat,
):
    """Generate report using rh_cloud plugin api's and verify its basic properties.

    :id: 8ead1ff6-a8f5-461b-9dd3-f50d96d6ed57

    :expectedresults:

        1. Report can be generated
        2. Report can be downloaded
        3. Report has non-zero size
        4. Report can be extracted
        5. JSON files inside report can be parsed
        6. metadata.json lists all and only slice JSON files in tar
        7. Host counts in metadata matches host counts in slices
        8. metadata contains source and foreman_rh_cloud_version keys.
        9. Assert Hostnames, IP addresses, infrastructure type, and installed packages
            are present in report.
        10. Assert that system_purpose_sla field is present in the inventory report.

    :CaseImportance: Critical

    :BZ: 1807829, 1926100, 1965234, 1824183, 1879453, 1845113

    :customerscenario: true
    """
    org = rhcloud_manifest_org
    virtual_host, baremetal_host = rhcloud_registered_hosts
    local_report_path = robottelo_tmp_dir.joinpath(f'{gen_alphanumeric()}_{org.id}.tar.xz')
    # Generate report
    module_target_sat.generate_inventory_report(org)
    # Download report
    module_target_sat.api.Organization(id=org.id).rh_cloud_download_report(
        destination=local_report_path
    )
    common_assertion(local_report_path)
    json_data = get_report_data(local_report_path)
    json_meta_data = get_report_metadata(local_report_path)
    # Verify that metadata contains source and foreman_rh_cloud_version keys.
    prefix = 'tfm-' if module_target_sat.os_version.major < 8 else ''
    package_version = module_target_sat.run(
        f'rpm -qa --qf "%{{VERSION}}" {prefix}rubygem-foreman_rh_cloud'
    ).stdout.strip()
    assert json_meta_data['source_metadata']['foreman_rh_cloud_version'] == str(package_version)
    assert json_meta_data['source'] == 'Satellite'
    # Verify Hostnames are present in report.
    hostnames = [host['fqdn'] for host in json_data['hosts']]
    assert virtual_host.hostname in hostnames
    assert baremetal_host.hostname in hostnames
    # Verify IP addresses are present in report.
    if module_target_sat.network_type == NetworkType.IPV6:
        # For IPv6 networks, check IPv6 addresses
        ip_addresses = [
            host['system_profile']['network_interfaces'][0]['ipv6_addresses'][0]
            for host in json_data['hosts']
        ]
        ipv6_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
        assert virtual_host.ip_addr in ip_addresses
        assert baremetal_host.ip_addr in ip_addresses
        assert virtual_host.ip_addr in ipv6_addresses
        assert baremetal_host.ip_addr in ipv6_addresses
    else:
        # For IPv4 networks, check IPv4 addresses
        ip_addresses = [
            host['system_profile']['network_interfaces'][0]['ipv4_addresses'][0]
            for host in json_data['hosts']
        ]
        ipv4_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
        assert virtual_host.ip_addr in ip_addresses
        assert baremetal_host.ip_addr in ip_addresses
        assert virtual_host.ip_addr in ipv4_addresses
        assert baremetal_host.ip_addr in ipv4_addresses
    # Verify infrastructure type.
    infrastructure_type = [
        host['system_profile']['infrastructure_type'] for host in json_data['hosts']
    ]
    assert 'physical' in infrastructure_type
    assert 'virtual' in infrastructure_type
    # Verify installed packages are present in report.
    all_host_profiles = [host['system_profile'] for host in json_data['hosts']]
    for host_profiles in all_host_profiles:
        assert 'installed_packages' in host_profiles
        assert len(host_profiles['installed_packages']) > 1
    # Verify that system_purpose_sla field is present in the inventory report.
    for host in json_data['hosts']:
        assert host['facts'][0]['facts']['system_purpose_role'] == 'test-role'
        assert host['facts'][0]['facts']['system_purpose_sla'] == 'Self-Support'
        assert host['facts'][0]['facts']['system_purpose_usage'] == 'test-usage'

</code_context>

<issue_to_address>
**issue (code-quality):** Hoist repeated code outside conditional statement [×2] ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
</issue_to_address>

### Comment 8
<location> `tests/foreman/ui/test_rhcloud_inventory.py:172` </location>
<code_context>
@pytest.mark.e2e
@pytest.mark.pit_server
@pytest.mark.pit_client
@pytest.mark.run_in_one_thread
@pytest.mark.usefixtures('setting_update')
@pytest.mark.parametrize(
    'setting_update',
    ['subscription_connection_enabled=true', 'subscription_connection_enabled=false'],
    indirect=True,
)
def test_rhcloud_inventory_e2e(
    inventory_settings,
    rhcloud_manifest_org,
    rhcloud_registered_hosts,
    module_target_sat,
    setting_update,
):
    """Generate report and verify its basic properties,
    also test with subscription_connection_enabled setting set to true and false.

    :id: 833bd61d-d6e7-4575-887a-9e0729d0fa76

    :parametrized: yes

    :customerscenario: true

    :expectedresults:

        1. Report can be generated
        2. Report can be downloaded
        3. Report has non-zero size
        4. Report can be extracted
        5. JSON files inside report can be parsed
        6. metadata.json lists all and only slice JSON files in tar
        7. Host counts in metadata matches host counts in slices
        8. Assert Hostnames, IP addresses, and installed packages are present in the report.

    :CaseImportance: Critical

    :BZ: 1807829, 1926100
    """
    subscription_setting = setting_update.value == 'true'
    org = rhcloud_manifest_org
    virtual_host, baremetal_host = rhcloud_registered_hosts
    with module_target_sat.ui_session() as session:
        session.organization.select(org_name=org.name)
        session.location.select(loc_name=DEFAULT_LOC)
        timestamp = (datetime.now(UTC) - timedelta(minutes=2)).strftime('%Y-%m-%d %H:%M')
        session.cloudinventory.generate_report(org.name)
        # wait_for_tasks report generation task to finish.
        wait_for(
            lambda: module_target_sat.api.ForemanTask()
            .search(
                query={
                    'search': f'label = ForemanInventoryUpload::Async::GenerateReportJob '
                    f'and started_at >= "{timestamp}"'
                }
            )[0]
            .result
            == 'success',
            timeout=400,
            delay=15,
            silent_failure=True,
            handle_exception=True,
        )
        report_path = session.cloudinventory.download_report(org.name)
        inventory_data = session.cloudinventory.read(org.name)
    # Verify that generated archive is valid.
    common_assertion(report_path, inventory_data, org, module_target_sat, subscription_setting)
    # Get report data for assertion
    json_data = get_report_data(report_path)
    # Verify that hostnames are present in the report.
    hostnames = [host['fqdn'] for host in json_data['hosts']]
    assert virtual_host.hostname in hostnames
    assert baremetal_host.hostname in hostnames
    # Verify that ip_addresses are present report.
    if module_target_sat.network_type == NetworkType.IPV6:
        # For IPv6 networks, check IPv6 addresses
        ip_addresses = [
            host['system_profile']['network_interfaces'][0]['ipv6_addresses'][0]
            for host in json_data['hosts']
        ]
        ipv6_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
        assert virtual_host.ip_addr in ip_addresses
        assert baremetal_host.ip_addr in ip_addresses
        assert virtual_host.ip_addr in ipv6_addresses
        assert baremetal_host.ip_addr in ipv6_addresses
    else:
        # For IPv4 networks, check IPv4 addresses
        ip_addresses = [
            host['system_profile']['network_interfaces'][0]['ipv4_addresses'][0]
            for host in json_data['hosts']
        ]
        ipv4_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
        assert virtual_host.ip_addr in ip_addresses
        assert baremetal_host.ip_addr in ip_addresses
        assert virtual_host.ip_addr in ipv4_addresses
        assert baremetal_host.ip_addr in ipv4_addresses
    # Verify that packages are included in report
    all_host_profiles = [host['system_profile'] for host in json_data['hosts']]
    for host_profiles in all_host_profiles:
        assert 'installed_packages' in host_profiles
        assert len(host_profiles['installed_packages']) > 1

</code_context>

<issue_to_address>
**issue (code-quality):** Hoist repeated code outside conditional statement [×2] ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
</issue_to_address>

### Comment 9
<location> `tests/foreman/ui/test_rhcloud_inventory.py:286` </location>
<code_context>
@pytest.mark.run_in_one_thread
def test_rh_cloud_inventory_settings(
    module_target_sat,
    inventory_settings,
    rhcloud_manifest_org,
    rhcloud_registered_hosts,
):
    """Test whether `Obfuscate host names`, `Obfuscate host ipv4 addresses`
        and `Exclude Packages` setting works as expected.

    :id: 3c3a36b6-6566-446b-b803-3f8f9aab2511

    :customerscenario: true

    :steps:

        1. Prepare machine and upload its data to Insights.
        2. Go to Insights > Inventory upload > enable “Obfuscate host names” setting.
        3. Go to Insights > Inventory upload > enable “Obfuscate host ip addresses” setting.
        4. Go to Insights > Inventory upload > enable “Exclude Packages” setting.
        5. Generate report after enabling the settings.
        6. Check if host names are obfuscated in generated reports.
        7. Check if hosts ipv6 or ipv4 addresses are obfuscated in generated reports.
        8. Check if packages are excluded from generated reports.
        9. Disable previous setting.
        10. Go to Administer > Settings > RH Cloud and enable "Obfuscate host names" setting.
        11. Go to Administer > Settings > RH Cloud and enable "Obfuscate IPs" setting.
        12. Go to Administer > Settings > RH Cloud and enable
            "Don't upload installed packages" setting.
        13. Generate report after enabling the setting.
        14. Check if host names are obfuscated in generated reports.
        15. Check if hosts ipv6 or ipv4 addresses are obfuscated in generated reports.
        16. Check if packages are excluded from generated reports.

    :expectedresults:
        1. Obfuscated host names in reports generated.
        2. Obfuscated host ipv6 or ipv4 addresses in generated reports.
        3. Packages are excluded from reports generated.

    :BZ: 1852594, 1889690, 1852594

    :CaseAutomation: Automated
    """
    org = rhcloud_manifest_org
    virtual_host, baremetal_host = rhcloud_registered_hosts
    with module_target_sat.ui_session() as session:
        session.organization.select(org_name=org.name)
        session.location.select(loc_name=DEFAULT_LOC)
        # Enable settings on inventory page.
        session.cloudinventory.update({'obfuscate_hostnames': True})
        session.cloudinventory.update({'obfuscate_ips': True})
        session.cloudinventory.update({'exclude_packages': True})
        timestamp = (datetime.now(UTC) - timedelta(minutes=2)).strftime('%Y-%m-%d %H:%M')
        session.cloudinventory.generate_report(org.name)
        # wait_for_tasks report generation task to finish.
        wait_for(
            lambda: module_target_sat.api.ForemanTask()
            .search(
                query={
                    'search': f'label = ForemanInventoryUpload::Async::GenerateReportJob '
                    f'and started_at >= "{timestamp}"'
                }
            )[0]
            .result
            == 'success',
            timeout=400,
            delay=15,
            silent_failure=True,
            handle_exception=True,
        )
        report_path = session.cloudinventory.download_report(org.name)
        inventory_data = session.cloudinventory.read(org.name)
        # Verify settings are enabled.
        assert inventory_data['obfuscate_hostnames'] is True
        assert inventory_data['obfuscate_ips'] is True
        assert inventory_data['exclude_packages'] is True
        # Verify that generated archive is valid.
        common_assertion(report_path, inventory_data, org, module_target_sat)
        # Get report data for assertion
        json_data = get_report_data(report_path)
        # Verify that hostnames are obfuscated from the report.
        hostnames = [host['fqdn'] for host in json_data['hosts']]
        assert virtual_host.hostname not in hostnames
        assert baremetal_host.hostname not in hostnames
        # Verify that ip_addresses are obfuscated from the report.
        if module_target_sat.network_type == NetworkType.IPV6:
            # For IPv6 networks, check IPv6 addresses
            ip_addresses = [
                host['system_profile']['network_interfaces'][0]['ipv6_addresses'][0]
                for host in json_data['hosts']
            ]
            ipv6_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
            assert virtual_host.ip_addr not in ip_addresses
            assert baremetal_host.ip_addr not in ip_addresses
            assert virtual_host.ip_addr not in ipv6_addresses
            assert baremetal_host.ip_addr not in ipv6_addresses
        else:
            # For IPv4 networks, check IPv4 addresses
            ip_addresses = [
                host['system_profile']['network_interfaces'][0]['ipv4_addresses'][0]
                for host in json_data['hosts']
            ]
            ipv4_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
            assert virtual_host.ip_addr not in ip_addresses
            assert baremetal_host.ip_addr not in ip_addresses
            assert virtual_host.ip_addr not in ipv4_addresses
            assert baremetal_host.ip_addr not in ipv4_addresses
        # Verify that packages are excluded from report
        all_host_profiles = [host['system_profile'] for host in json_data['hosts']]
        for host_profiles in all_host_profiles:
            assert 'installed_packages' not in host_profiles
        # Disable settings on inventory page.
        session.cloudinventory.update({'obfuscate_hostnames': False})
        session.cloudinventory.update({'obfuscate_ips': False})
        session.cloudinventory.update({'exclude_packages': False})
        # Enable settings, the one on the main settings page.
        module_target_sat.update_setting('obfuscate_inventory_hostnames', True)
        module_target_sat.update_setting('obfuscate_inventory_ips', True)
        module_target_sat.update_setting('exclude_installed_packages', True)
        timestamp = (datetime.now(UTC) - timedelta(minutes=2)).strftime('%Y-%m-%d %H:%M')
        session.cloudinventory.generate_report(org.name)
        # wait_for_tasks report generation task to finish.
        wait_for(
            lambda: module_target_sat.api.ForemanTask()
            .search(
                query={
                    'search': f'label = ForemanInventoryUpload::Async::GenerateReportJob '
                    f'and started_at >= "{timestamp}"'
                }
            )[0]
            .result
            == 'success',
            timeout=400,
            delay=15,
            silent_failure=True,
            handle_exception=True,
        )
        report_path = session.cloudinventory.download_report(org.name)
        inventory_data = session.cloudinventory.read(org.name)
        # Verify settings are enabled.
        assert inventory_data['obfuscate_hostnames'] is True
        assert inventory_data['obfuscate_ips'] is True
        assert inventory_data['exclude_packages'] is True
        # Get report data for assertion
        json_data = get_report_data(report_path)
        # Verify that hostnames are obfuscated from the report.
        hostnames = [host['fqdn'] for host in json_data['hosts']]
        assert virtual_host.hostname not in hostnames
        assert baremetal_host.hostname not in hostnames
        # Verify that ip_addresses are obfuscated from the report.
        if module_target_sat.network_type == NetworkType.IPV6:
            # For IPv6 networks, check IPv6 addresses
            ip_addresses = [
                host['system_profile']['network_interfaces'][0]['ipv6_addresses'][0]
                for host in json_data['hosts']
            ]
            ipv6_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
            assert virtual_host.ip_addr not in ip_addresses
            assert baremetal_host.ip_addr not in ip_addresses
            assert virtual_host.ip_addr not in ipv6_addresses
            assert baremetal_host.ip_addr not in ipv6_addresses
        else:
            # For IPv4 networks, check IPv4 addresses
            ip_addresses = [
                host['system_profile']['network_interfaces'][0]['ipv4_addresses'][0]
                for host in json_data['hosts']
            ]
            ipv4_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
            assert virtual_host.ip_addr not in ip_addresses
            assert baremetal_host.ip_addr not in ip_addresses
            assert virtual_host.ip_addr not in ipv4_addresses
            assert baremetal_host.ip_addr not in ipv4_addresses
        # Verify that packages are excluded from report
        all_host_profiles = [host['system_profile'] for host in json_data['hosts']]
        for host_profiles in all_host_profiles:
            assert 'installed_packages' not in host_profiles

</code_context>

<issue_to_address>
**issue (code-quality):** Hoist repeated code outside conditional statement [×4] ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_rhcloud_inventory.py -k "test_rh_cloud_inventory_settings"

@ColeHiggins2 ColeHiggins2 marked this pull request as ready for review November 20, 2025 18:27
@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_rhcloud_inventory.py -k "test_rh_cloud_inventory_settings"

Copy link
Member Author

@ColeHiggins2 ColeHiggins2 left a comment

Choose a reason for hiding this comment

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

test passes in prt. not sure why its not showing here
15:30:10 ========= 8 passed, 40 deselected, 348 warnings in 6353.08s (1:45:53) ==========

Copy link
Contributor

@jnagare-redhat jnagare-redhat left a comment

Choose a reason for hiding this comment

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

Ack LGTM

Comment on lines 94 to 115
if module_target_sat.network_type == NetworkType.IPV6:
# For IPv6 networks, check IPv6 addresses
ip_addresses = [
host['system_profile']['network_interfaces'][0]['ipv6_addresses'][0]
for host in json_data['hosts']
]
ipv6_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
assert virtual_host.ip_addr in ip_addresses
assert baremetal_host.ip_addr in ip_addresses
assert virtual_host.ip_addr in ipv6_addresses
assert baremetal_host.ip_addr in ipv6_addresses
else:
# For IPv4 networks, check IPv4 addresses
ip_addresses = [
host['system_profile']['network_interfaces'][0]['ipv4_addresses'][0]
for host in json_data['hosts']
]
ipv4_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
assert virtual_host.ip_addr in ip_addresses
assert baremetal_host.ip_addr in ip_addresses
assert virtual_host.ip_addr in ipv4_addresses
assert baremetal_host.ip_addr in ipv4_addresses
Copy link
Contributor

Choose a reason for hiding this comment

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

@ColeHiggins2 Could you please try it this way, I find it more compact. Should work too imo:

Suggested change
if module_target_sat.network_type == NetworkType.IPV6:
# For IPv6 networks, check IPv6 addresses
ip_addresses = [
host['system_profile']['network_interfaces'][0]['ipv6_addresses'][0]
for host in json_data['hosts']
]
ipv6_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
assert virtual_host.ip_addr in ip_addresses
assert baremetal_host.ip_addr in ip_addresses
assert virtual_host.ip_addr in ipv6_addresses
assert baremetal_host.ip_addr in ipv6_addresses
else:
# For IPv4 networks, check IPv4 addresses
ip_addresses = [
host['system_profile']['network_interfaces'][0]['ipv4_addresses'][0]
for host in json_data['hosts']
]
ipv4_addresses = [host['ip_addresses'][0] for host in json_data['hosts']]
assert virtual_host.ip_addr in ip_addresses
assert baremetal_host.ip_addr in ip_addresses
assert virtual_host.ip_addr in ipv4_addresses
assert baremetal_host.ip_addr in ipv4_addresses
# Dynamic key determination based on the network type
is_ipv6 = module_target_sat.network_type == NetworkType.IPV6
key = 'ipv6_addresses' if is_ipv6 else 'ipv4_addresses'
ip_addresses = {
host['system_profile']['network_interfaces'][0][key][0]
for host in json_data['hosts']
}
ips = {host['ip_addresses'][0] for host in json_data['hosts']}
for host_ip in (virtual_host.ip_addr, baremetal_host.ip_addr):
assert host_ip in ip_addresses
assert host_ip in ips

Copy link
Contributor

Choose a reason for hiding this comment

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

yes agreed. looks more compact to me as well.

@ColeHiggins2 ColeHiggins2 force-pushed the update-ip-address-check-ipv6 branch from ad29e31 to 1038c28 Compare November 21, 2025 19:28
@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_rhcloud_inventory.py -k "test_rh_cloud_inventory_settings"

Copy link
Contributor

@synkd synkd left a comment

Choose a reason for hiding this comment

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

Looks like something is off in the logic of the new version, as all tests are now failing PRT with assertions on mismatched IP addresses.

However, I'm ACKing as Cole's original version passed PRT. @ColeHiggins2 I will leave it up to you whether to fix the more concise version or revert to your original version.

@ColeHiggins2 ColeHiggins2 force-pushed the update-ip-address-check-ipv6 branch from 1038c28 to c137fa6 Compare November 25, 2025 18:41
@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_rhcloud_inventory.py -k "test_rh_cloud_inventory_settings"

@ogajduse
Copy link
Member

ogajduse commented Dec 2, 2025

trigger: test-robottelo
pytest: tests/foreman/ui/test_rhcloud_inventory.py -k "test_rh_cloud_inventory_settings"
network_type: ipv6

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.

ContentHost.ip_addr does not work reliably and currently only returns the IPv4 address. Therefore, the test cannot rely on it to return an IPv6 address.

@property
def ip_addr(self):
    ipv4, *ipv6 = self.execute('hostname -I').stdout.split()
    return ipv4
[ins] In [1]: hostname_i_output = "10.202.145.224 10.40.2.190 2620:52:0:2802:5632:7419:bb83:80b2"

[ins] In [2]: ipv4, *ipv6 = hostname_i_output.split()

[ins] In [3]: ipv4
Out[3]: '10.202.145.224'

[ins] In [4]: ipv6
Out[4]: ['10.40.2.190', '2620:52:0:2802:5632:7419:bb83:80b2']

@ColeHiggins2 ColeHiggins2 force-pushed the update-ip-address-check-ipv6 branch from c137fa6 to 13f8e69 Compare December 2, 2025 15:03
@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_rhcloud_inventory.py -k "test_rh_cloud_inventory_settings"

@ColeHiggins2
Copy link
Member Author

ColeHiggins2 commented Dec 2, 2025

ContentHost.ip_addr does not work reliably and currently only returns the IPv4 address. Therefore, the test cannot rely on it to return an IPv6 address.

@property
def ip_addr(self):
    ipv4, *ipv6 = self.execute('hostname -I').stdout.split()
    return ipv4
[ins] In [1]: hostname_i_output = "10.202.145.224 10.40.2.190 2620:52:0:2802:5632:7419:bb83:80b2"

[ins] In [2]: ipv4, *ipv6 = hostname_i_output.split()

[ins] In [3]: ipv4
Out[3]: '10.202.145.224'

[ins] In [4]: ipv6
Out[4]: ['10.40.2.190', '2620:52:0:2802:5632:7419:bb83:80b2']

What do you recommend I do then? @ogajduse

@ogajduse
Copy link
Member

ogajduse commented Dec 5, 2025

ContentHost.ip_addr does not work reliably and currently only returns the IPv4 address. Therefore, the test cannot rely on it to return an IPv6 address.

@property
def ip_addr(self):
    ipv4, *ipv6 = self.execute('hostname -I').stdout.split()
    return ipv4
[ins] In [1]: hostname_i_output = "10.202.145.224 10.40.2.190 2620:52:0:2802:5632:7419:bb83:80b2"

[ins] In [2]: ipv4, *ipv6 = hostname_i_output.split()

[ins] In [3]: ipv4
Out[3]: '10.202.145.224'

[ins] In [4]: ipv6
Out[4]: ['10.40.2.190', '2620:52:0:2802:5632:7419:bb83:80b2']

What do you recommend I do then? @ogajduse

I'd suggest rewriting the ip_addr property. Here is something I put together with Claude's assistance. This fixes the IPv6 issue by using ip -j addr show for reliable parsing and makes the property network-type aware. It aims to be backward compatible with existing tests.

This is just a suggestion, though.

    @property
    def ip_addr(self):
        """Return the primary IP address.

        Returns the first global-scope IP address based on network_type:
        - IPV4: Returns IPv4 address only (no fallback)
        - IPV6: Returns IPv6 address only (no fallback)
        - DUALSTACK: Returns IPv6 address (falls back to IPv4 if no IPv6 available)
        - Default (no network_type): Returns IPv4 address (falls back to IPv6 if no IPv4 available)

        Excludes loopback and link-local addresses.
        """
        addrs = self.ip_addrs

        # Check if network_type is available
        if hasattr(self, 'network_type'):
            if self.network_type == NetworkType.IPV6:
                # IPV6: Return IPv6 only, no fallback
                return addrs['ipv6'][0] if addrs['ipv6'] else None
            if self.network_type == NetworkType.IPV4:
                # IPV4: Return IPv4 only, no fallback
                return addrs['ipv4'][0] if addrs['ipv4'] else None
            if self.network_type == NetworkType.DUALSTACK:
                # DUALSTACK: Prefer IPv6, fall back to IPv4
                if addrs['ipv6']:
                    return addrs['ipv6'][0]
                if addrs['ipv4']:
                    return addrs['ipv4'][0]
                return None

        # Default (no network_type): Prefer IPv4, fall back to IPv6 for backward compatibility
        if addrs['ipv4']:
            return addrs['ipv4'][0]
        if addrs['ipv6']:
            return addrs['ipv6'][0]
        return None

    @property
    def ip_addrs(self):
        """Return all IP addresses as a dictionary.

        Uses the 'ip' command with JSON output.
        Only returns global-scope addresses (excludes loopback and link-local).

        Returns:
            dict: Dictionary with 'ipv4' and 'ipv6' keys, each containing a list of addresses
        """
        result = self.execute('ip -json address show')
        if result.status != 0:
            # Fall back to --all-ip-addresses if ip command fails
            addrs = self.execute('hostname --all-ip-addresses').stdout.split()
            ipv4_addrs = [addr for addr in addrs if ':' not in addr]
            ipv6_addrs = [addr for addr in addrs if ':' in addr]
            return {'ipv4': ipv4_addrs, 'ipv6': ipv6_addrs}

        ipv4_addrs = []
        ipv6_addrs = []

        try:
            interfaces = json.loads(result.stdout)
            for iface in interfaces:
                for addr_info in iface.get('addr_info', []):
                    scope = addr_info.get('scope', '')
                    local_addr = addr_info.get('local', '')
                    family = addr_info.get('family', '')

                    # Only include global scope addresses (skip host/link scopes)
                    if scope == 'global' and local_addr:
                        if family == 'inet':
                            ipv4_addrs.append(local_addr)
                        elif family == 'inet6':
                            ipv6_addrs.append(local_addr)
        except (json.JSONDecodeError, KeyError):
            # If JSON parsing fails, fall back to hostname --all-ip-addresses
            addrs = self.execute('hostname --all-ip-addresses').stdout.split()
            ipv4_addrs = [addr for addr in addrs if ':' not in addr]
            ipv6_addrs = [addr for addr in addrs if ':' in addr]

        return {'ipv4': ipv4_addrs, 'ipv6': ipv6_addrs}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.17.z 6.18.z Introduced in or relating directly to Satellite 6.18 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants