Skip to content

Address Pending Comments from Dnf5 Original PR #355

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.

Sign up for GitHub

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

Open
yashnap wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Address Pending Comments from Dnf5 Original PR #355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5898393
Address Pending Comments
yashnap Jun 22, 2026
ae0cbce
Merge branch 'master' into dnf5_pending_comments
yashnap Jun 22, 2026
735c1c9
Address Copilot review
yashnap Jun 22, 2026
b8c8fec
Merge branch 'dnf5_pending_comments' of https://github.com/Azure/Linu…
yashnap Jun 22, 2026
8adaa58
Address Code Review comments #1
yashnap Jun 22, 2026
1c9c1ff
Address Code Review comments #1
yashnap Jun 22, 2026
e8dd90b
Address Code Review comment #2
yashnap Jun 23, 2026
3f55363
Separate out methods
yashnap Jun 23, 2026
460f146
Merge branch 'master' into dnf5_pending_comments
yashnap Jul 21, 2026
0ebb826
Fix merge conflicts
yashnap Jul 21, 2026
4b27687
Address code review
yashnap Jul 24, 2026
d416f37
Address code review
yashnap Jul 27, 2026
f948f05
Remove unused method
yashnap Jul 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/core/src/bootstrap/EnvLayer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ def is_distro_rhel_10(self, distro_name):
""" Checks if the current distro is RHEL 10 """
return self.__is_matching_distro_and_version(distro_name, Constants.RED_HAT, version_to_match=10)

def __get_dnf_version(self):
code, out = self.run_command_output('dnf --version', False, False)
# Output : dnf5 version 5.2.18.0
if code != 0 or not out:
return code, out, None
version = str(out).split()[-1].split('.')[0]
return code, out, version

def get_package_manager(self):
# type: () -> str
""" Detects package manager type """
Expand All @@ -99,11 +107,14 @@ def get_package_manager(self):

# Check for Azure Linux 4 or Above( uses dnf5)
if self.is_distro_azure_linux_4(str(os_name)):
code, out = self.run_command_output('which dnf', False, False)
if code == 0:
code, out, version = self.__get_dnf_version()

@rane-rajasi rane-rajasi Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't __get_dnf_version() i.e. command 'dnf --version' return a non zero exit code if dnf is not available? What is the need for a separate __is_dnf_available() then?

if could simply be:

Check for Azure Linux 4 or Above( uses dnf5)

    if self.is_distro_azure_linux_4(str(os_name)):
        code, out, version = self.__get_dnf_version()
        if code == 0 and version == 5:
            return Constants.DNF5
        else:
            print("Error: Expected package manager dnf5 not found on this Azure Linux4 VM.")
            return str()

@yashnap yashnap Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I split the checks intentionally to address the diagnosability concerns from the earlier review comment: #355 (comment)

With only dnf --version, multiple failure modes get folded into the same code path:

  1. dnf is not installed / not found
  2. dnf is present but returns version 4
  3. dnf is present but returns version 6
  4. dnf --version itself fails for some other reason

By checking __is_dnf_available() first, we can provide a specific error when dnf is missing and __get_dnf_version() is only responsible for version-related validation. While dnf --version alone can be used to detect all cases, the separate availability check allows to distinguish "dnf missing" from "dnf present but invalid/unexpected"

@rane-rajasi rane-rajasi Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see these 3 points in Koshy's comment:

an error
4 - surprising but clear
6 - something changed

All of these points can still be addressed with just running one command 'dnf --version' because this is what determines if the code continues.
If 'which dnf' fails OR if 'which dnf' succeeds and 'dnf --version', both of these indicate a problem in the VM which needs to be logged for the VM owner to address.

if self.is_distro_azure_linux_4(str(os_name)):
    code, out, version = self.__get_dnf_version()
    if code == 0 and version == 5:
        return Constants.DNF5
    elif code=0 and version !=5:
        print("Error: Expected dnf version not found on this Azure Linux4 VM. [Expected={0}][Found={1}]".format("5", str(version))
        return str()
    else:
        print("Error: Expected package manager dnf5 not found on this Azure Linux4 VM.")
        return str()

@yashnap yashnap Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If VM owner needs to address it then it makes sense. Thanks
Updated the code.

if code == 0 and version == '5':
return Constants.DNF5
elif code == 0 and version != '5':
Comment on lines 109 to +113
print("Error: Expected dnf version not found on this Azure Linux4 VM. [Expected={0}][Found={1}]".format("5", str(version)))
return str()
else:
print("Error: Expected package manager dnf5 not found on this Azure Linux4 VM.")
print("Error: Expected package manager dnf5 not found on this Azure Linux4 VM")
return str()

# Check for Azure Linux (3 and below use TDNF)
Expand Down
6 changes: 4 additions & 2 deletions src/core/src/package_managers/Dnf5PackageManager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,9 +263,9 @@ def get_dependent_list(self, packages):
code, output = self.env_layer.run_command_output(cmd, False, False)
self.composite_logger.log_verbose("[DNF5] Dependency simulation. [Command={0}][Code={1}]".format(cmd, str(code)))
if code not in self.dnf5_simulation_valid_exit_codes:
self.composite_logger.log_error("[DNF5] Unexpected failure. [Command={0}][Code={1}][Output={2}]".format(cmd, str(code), output))
self.composite_logger.log_error("[DNF5] Unexpected failure during dependency simulation. [Command={0}][Code={1}][Output={2}]".format(cmd, str(code), output))
error_msg = "DNF5 dependency simulation failed. Investigate and resolve unexpected return code({0}) from package manager on command: {1} ".format(str(code), cmd)
self.status_handler.add_error_to_status(error_msg, Constants.PatchOperationErrorCodes.DEFAULT_ERROR)
self.status_handler.add_error_to_status(error_msg, Constants.PatchOperationErrorCodes.PACKAGE_MANAGER_FAILURE)
raise Exception(error_msg, "[{0}]".format(Constants.ERROR_ADDED_TO_STATUS))

dependencies = self.extract_dependencies(output, packages)
Expand Down Expand Up @@ -727,3 +727,5 @@ def is_reboot_required_before_cert_update(self):
""" Checks if a reboot is required before updating certificates """
return False
# endregion


Comment on lines 729 to +731
94 changes: 85 additions & 9 deletions src/core/tests/Test_EnvLayer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ def mock_platform_system_windows(self):
def mock_linux_distribution(self):
return ['test', 'test', 'test']

def mock_linux_distribution_to_return_azure_linux_4(self):
return ['Microsoft Azure Linux', '4.0', '']

def mock_linux_distribution_to_return_azure_linux_3(self):
return ['Microsoft Azure Linux', '3.0', '']

Expand All @@ -71,6 +74,38 @@ def mock_run_command_for_tdnf(self, cmd, no_output=False, chk_err=False):
return 0, ''
return -1, ''

def mock_run_command_for_dnf5(self, cmd, no_output=False, chk_err=False):
if "dnf --version" in cmd:
return 0, 'dnf5 version 5.2.18.0'
return -1, ''

def mock_run_command_for_dnf_not_found(self, cmd, no_output=False, chk_err=False):
return -1, ''

def mock_run_command_for_dnf_wrong_version(self, cmd, no_output=False, chk_err=False):
return self.mock_run_command_for_dnf_version_command(cmd, no_output, chk_err, "wrong_version")

def mock_run_command_for_dnf_version_command_failure(self, cmd, no_output=False, chk_err=False):
return self.mock_run_command_for_dnf_version_command(cmd, no_output, chk_err, "version_command_failure")

def mock_run_command_for_dnf_version_command(self, cmd, no_output=False, chk_err=False, usecase="success"):
code = -1
out = ""
if "dnf --version" in cmd:
if usecase == "wrong_version":
code = 0
out = "dnf version 4.14.0"
elif usecase == "version_command_failure":
code = -1
out = "dnf version command failure"
else:
code = 0
out = "dnf version 5.2.6"
return code, out

def mock_distro_os_release_attr_return_azure_linux_4(self, attribute):
return '4.0.0'

def mock_distro_os_release_attr_return_azure_linux_3(self, attribute):
return '3.0.0'

Expand Down Expand Up @@ -126,20 +161,26 @@ def test_get_package_manager(self):
self.backup_distro_os_release_attr = distro.os_release_attr

test_input_output_table = [
[self.mock_run_command_for_apt, self.mock_linux_distribution, Constants.APT],
[self.mock_run_command_for_tdnf, self.mock_linux_distribution_to_return_azure_linux_3, Constants.TDNF],
[self.mock_run_command_for_yum, self.mock_linux_distribution_to_return_azure_linux_3, str()], # check for Azure Linux machine which does not have tdnf
[self.mock_run_command_for_tdnf, self.mock_linux_distribution_to_return_azure_linux_2, Constants.TDNF],
[self.mock_run_command_for_yum, self.mock_linux_distribution, Constants.YUM],
[self.mock_run_command_for_zypper, self.mock_linux_distribution, Constants.ZYPPER],
[lambda cmd, no_output, chk_err: (-1, ''), self.mock_linux_distribution, str()], # no matches for any package manager
[self.mock_run_command_for_apt, self.mock_linux_distribution, self.mock_distro_os_release_attr_return_none, Constants.APT],
[self.mock_run_command_for_dnf5, self.mock_linux_distribution_to_return_azure_linux_4, self.mock_distro_os_release_attr_return_azure_linux_4, Constants.DNF5],
[self.mock_run_command_for_tdnf, self.mock_linux_distribution_to_return_azure_linux_3, self.mock_distro_os_release_attr_return_azure_linux_3, Constants.TDNF],
[self.mock_run_command_for_yum, self.mock_linux_distribution_to_return_azure_linux_3, self.mock_distro_os_release_attr_return_none, str()], # check for Azure Linux machine which does not have tdnf
[self.mock_run_command_for_tdnf, self.mock_linux_distribution_to_return_azure_linux_2, self.mock_distro_os_release_attr_return_azure_linux_2, Constants.TDNF],
[self.mock_run_command_for_yum, self.mock_linux_distribution, self.mock_distro_os_release_attr_return_none, Constants.YUM],
[self.mock_run_command_for_zypper, self.mock_linux_distribution, self.mock_distro_os_release_attr_return_none, Constants.ZYPPER],
[lambda cmd, no_output, chk_err: (-1, ''), self.mock_linux_distribution, self.mock_distro_os_release_attr_return_none, str()], # no matches for any package manager
[self.mock_run_command_for_dnf_not_found, self.mock_linux_distribution_to_return_azure_linux_4, self.mock_distro_os_release_attr_return_azure_linux_4, str()],
[self.mock_run_command_for_dnf_wrong_version, self.mock_linux_distribution_to_return_azure_linux_4, self.mock_distro_os_release_attr_return_azure_linux_4, str()],
[self.mock_run_command_for_dnf_version_command_failure, self.mock_linux_distribution_to_return_azure_linux_4, self.mock_distro_os_release_attr_return_azure_linux_4, str()],
[self.mock_run_command_for_dnf_version_command, self.mock_linux_distribution_to_return_azure_linux_4, self.mock_distro_os_release_attr_return_azure_linux_4, Constants.DNF5],
]

for row in test_input_output_table:
self.envlayer.run_command_output = row[0]
self.envlayer.platform.linux_distribution = row[1]
distro.os_release_attr = row[2]
package_manager = self.envlayer.get_package_manager()
self.assertTrue(package_manager is row[2])
self.assertEqual(package_manager, row[3])

# test for Windows
platform.system = self.mock_platform_system_windows
Expand All @@ -148,6 +189,7 @@ def test_get_package_manager(self):
# restore original methods
self.envlayer.run_command_output = self.backup_run_command_output
self.envlayer.platform.linux_distribution = self.backup_linux_distribution
distro.os_release_attr = self.backup_distro_os_release_attr
platform.system = self.backup_platform_system

def test_is_distro_azure_linux_3(self):
Expand All @@ -168,6 +210,23 @@ def test_is_distro_azure_linux_3(self):
# restore original methods
distro.os_release_attr = self.backup_envlayer_distro_os_release_attr

def test_is_distro_azure_linux_4(self):
self.backup_envlayer_distro_os_release_attr = distro.os_release_attr

test_input_output_table = [
[self.mock_linux_distribution_to_return_azure_linux_4, self.mock_distro_os_release_attr_return_azure_linux_4, True],
[self.mock_linux_distribution_to_return_azure_linux_4, self.mock_distro_os_release_attr_return_none, False],
]

for row in test_input_output_table:
distro_name = row[0]()[0] # Extract distro name from tuple (first element)
distro.os_release_attr = row[1]
result = self.envlayer.is_distro_azure_linux_4(distro_name)
self.assertEqual(result, row[2])

# restore original methods
distro.os_release_attr = self.backup_envlayer_distro_os_release_attr

def test_detect_confidential_vm_by_fde(self):
backup_run_command_output = self.envlayer.run_command_output
backup_read_with_retry = self.envlayer.file_system.read_with_retry
Expand Down Expand Up @@ -255,7 +314,7 @@ def test_platform(self):
self.envlayer.platform.cpu_arch()
self.envlayer.platform.vm_name()

def test_get_package_manager_azure_linux_4_and_rhel10_not_supported(self):
def test_get_package_manager_rhel10_not_supported(self):
"""Test for RHEL 10 log unsupported message"""
self.backup_platform_system = platform.system
self.backup_linux_distribution = self.envlayer.platform.linux_distribution
Expand All @@ -280,6 +339,23 @@ def test_get_package_manager_azure_linux_4_and_rhel10_not_supported(self):
# restore
self.__restore_mocks()

def test_mock_command_fallback_paths(self):
"""Test that mock commands return -1 for unexpected commands"""
code, out = self.mock_run_command_for_apt('which apt')

@rane-rajasi rane-rajasi Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why a test for mocked functions?

@yashnap yashnap Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added this to have full coverage. One of Koshy's comments from Teams thread. https://teams.microsoft.com/l/message/19:a30b906bea084801ae7406e964dc929e@thread.skype/1782148751374?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=ff48c73a-32a8-4ce4-b51a-322055b1633c&parentMessageId=1782148751374&teamName=GuestPatching&channelName=%F0%9F%91%80%20PR%20Reviews&createdTime=1782148751374&ngc=true

self.assertEqual(code, -1)

code, out = self.mock_run_command_for_dnf5('which not-dnf')
self.assertEqual(code, -1)

code, out = self.mock_run_command_for_dnf_version_command('dnf --v', "wrong_version")
self.assertEqual(code, -1)

code, out = self.mock_run_command_for_dnf_version_command('dnf --v', "version_command_failure")
self.assertEqual(code, -1)
Comment on lines +350 to +354

code, out = self.mock_run_command_for_tdnf('which not-tdnf')
self.assertEqual(code, -1)

def __restore_mocks(self):
"""Restore backed up mocks to their original state"""
distro.os_release_attr = self.backup_distro_os_release_attr
Expand Down
Loading