Skip to content

Address Pending Comments from Dnf5 Original PR#355

Open
yashnap wants to merge 8 commits into
masterfrom
dnf5_pending_comments
Open

Address Pending Comments from Dnf5 Original PR#355
yashnap wants to merge 8 commits into
masterfrom
dnf5_pending_comments

Conversation

@yashnap

@yashnap yashnap commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Copilot AI review requested due to automatic review settings June 22, 2026 16:43

Copilot AI left a comment

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.

Pull request overview

This PR updates Azure Linux 4/DNF5-related behavior and unit tests to address outstanding review comments from the original DNF5 enablement work, focusing on package-manager detection and improved error/status reporting.

Changes:

  • Update Azure Linux 4 package manager detection logic in EnvLayer.get_package_manager().
  • Refine DNF5 dependency-simulation error logging and status error code reporting.
  • Extend Test_EnvLayer coverage to include Azure Linux 4 and updated detection paths.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/core/tests/Test_EnvLayer.py Adds Azure Linux 4 coverage and adjusts mocks/table-driven tests for package manager detection.
src/core/src/package_managers/Dnf5PackageManager.py Improves dependency-simulation failure logging and uses a more specific package-manager failure error code.
src/core/src/bootstrap/EnvLayer.py Updates Azure Linux 4 package manager detection logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
Comment on lines 100 to 104
# 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

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.

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?

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.

Comment on lines +77 to +80
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, ''

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

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

Comment thread src/core/tests/Test_EnvLayer.py Outdated
Comment thread src/core/tests/Test_EnvLayer.py Outdated
@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.05%. Comparing base (9608cd9) to head (3f55363).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   93.99%   94.05%   +0.05%     
==========================================
  Files         107      107              
  Lines       19810    19888      +78     
==========================================
+ Hits        18621    18706      +85     
+ Misses       1189     1182       -7     
Flag Coverage Δ
python27 94.05% <100.00%> (+0.05%) ⬆️
python312 94.05% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[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],
[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

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.

Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
version = str(out).split()[-1]
if version.startswith('5'):
return Constants.DNF5
print("Error: Expected dnf version 5 not found on this Azure Linux4 VM.")

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.

Yes, and what did dnf --version actually return.

Imagine it returns:

  • an error
  • 4 - surprising but clear
  • 6 - something changed

As it is here, it's not possible to distinguish between these 3 states (or more).

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 logic to log version if not 5 and generic error message for dnf --version command error with logs.

@yashnap yashnap force-pushed the dnf5_pending_comments branch from 98e2a92 to 3f55363 Compare June 23, 2026 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants