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 10 commits into
base: master
Choose a base branch
from
+129 −16
Open

Address Pending Comments from Dnf5 Original PR #355

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 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
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
4 changes: 2 additions & 2 deletions src/core/src/bootstrap/EnvLayer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ 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 = self.run_command_output('dnf --version', False, False)
if code == 0 and out and str(out).strip().startswith('5'):
return Constants.DNF5

@yashnap yashnap Jun 22, 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.

Combined which dnf + version check into one. Currently we have a separate check for dnf4 use-case( RHEL 10) in our case. Keeping this as is. If there is no dnf present it will error out.

@kjohn-msft kjohn-msft Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not diagnosable as-is in the error condition. If we see this error line provided to you: "Error: Expected package manager dnf5 not found on this Azure Linux4 VM." -- without getting access to the VM, what do we know about what went wrong?

@yashnap yashnap Jun 22, 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.

Make sense. I've made changes to separate the check and add logging for easier diagnosis without having access o the VM.

else:
print("Error: Expected package manager dnf5 not found on this Azure Linux4 VM.")
Expand Down
4 changes: 2 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
47 changes: 39 additions & 8 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,14 @@ 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 cmd.find("dnf --version") > -1:
return 0, '5.2.0'
return -1, ''
Comment on lines +77 to +82

@yashnap yashnap Jun 22, 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.

Since this is a mock, no need for actual path. Keeping it as is

@yashnap yashnap Jun 23, 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 path for which dnf command


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 All @@ -96,18 +107,20 @@ 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, Constants.APT, self.mock_distro_os_release_attr_return_none],
[self.mock_run_command_for_dnf5, self.mock_linux_distribution_to_return_azure_linux_4, Constants.DNF5, self.mock_distro_os_release_attr_return_azure_linux_4],
[self.mock_run_command_for_tdnf, self.mock_linux_distribution_to_return_azure_linux_3, Constants.TDNF, self.mock_distro_os_release_attr_return_azure_linux_3],
[self.mock_run_command_for_yum, self.mock_linux_distribution_to_return_azure_linux_3, str(), self.mock_distro_os_release_attr_return_none], # 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_distro_os_release_attr_return_azure_linux_3],
Comment thread
Copilot marked this conversation as resolved.
Outdated
[self.mock_run_command_for_yum, self.mock_linux_distribution, Constants.YUM, self.mock_distro_os_release_attr_return_none],
[self.mock_run_command_for_zypper, self.mock_linux_distribution, Constants.ZYPPER, self.mock_distro_os_release_attr_return_none],
[lambda cmd, no_output, chk_err: (-1, ''), self.mock_linux_distribution, str(), self.mock_distro_os_release_attr_return_none], # no matches for any package manager

@yashnap yashnap Jun 22, 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 4th column to address the following issue:

  1. After adding DNF5 UT change
    2.. Calls get_package_manager() → is_distro_azure_linux_4()
  2. is_distro_azure_linux_4() calls __try_get_major_version()
  3. Since we don't set the os_release_attr in the method , it defaults to None and major = None → is_distro_azure_linux_4() returns False
  4. This leads to is_distro_azure_linux_4 returning false and call goes to tdnf if check and fails

  • Added 4th column to have os_release_attribute for each so version can be set.

  • Let me know your thoughts on this.

]

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[3]
package_manager = self.envlayer.get_package_manager()
self.assertTrue(package_manager is row[2])
Comment thread
Copilot marked this conversation as resolved.
Outdated

Expand All @@ -118,6 +131,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 @@ -138,6 +152,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_filesystem(self):
# only validates if these invocable without exceptions
backup_retry_count = Constants.MAX_FILE_OPERATION_RETRY_COUNT
Expand All @@ -152,7 +183,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 Down
Loading