Address Pending Comments from Dnf5 Original PR - #355
Conversation
There was a problem hiding this comment.
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_EnvLayercoverage 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.
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Make sense. I've made changes to separate the check and add logging for easier diagnosis without having access o the VM.
| 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, '' |
There was a problem hiding this comment.
Since this is a mock, no need for actual path. Keeping it as is
There was a problem hiding this comment.
Added path for which dnf command
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #355 +/- ##
==========================================
+ Coverage 94.10% 94.15% +0.05%
==========================================
Files 109 109
Lines 20642 20700 +58
==========================================
+ Hits 19425 19490 +65
+ Misses 1217 1210 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…xPatchExtension into dnf5_pending_comments
98e2a92 to
3f55363
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| code, out, version = self.__get_dnf_version() | ||
| if version: | ||
| if version.startswith('5'): |
There was a problem hiding this comment.
I'm wondering if we should be smarter about the version comparison here. I know this is highly unlikely, but if we get up to dnf50 (i know, i know), then this would return true. Can we instead split on periods, then compare the first part to 5 directly, rather than using starts with?
There was a problem hiding this comment.
We can do that. I'll address this in the next iteration.
| print("Error: Expected package manager dnf not found on this Azure Linux4 VM.") | ||
| return str() | ||
|
|
||
| code, out, version = self.__get_dnf_version() |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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:
- dnf is not installed / not found
- dnf is present but returns version 4
- dnf is present but returns version 6
- 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"
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
If VM owner needs to address it then it makes sense. Thanks
Updated the code.
|
#355 (comment) - Fixed this, Somehow I am not able to reply to this and can't see it inline |
| [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()], | ||
| [lambda cmd, no_output, chk_err=False: self.mock_run_command_for_dnf_version_command(cmd, "wrong_version"), self.mock_linux_distribution_to_return_azure_linux_4, self.mock_distro_os_release_attr_return_azure_linux_4, str()], | ||
| [lambda cmd, no_output, chk_err=False: self.mock_run_command_for_dnf_version_command(cmd,"version_command_failure"), self.mock_linux_distribution_to_return_azure_linux_4, self.mock_distro_os_release_attr_return_azure_linux_4, str()] |
There was a problem hiding this comment.
Why the use of lambda here instead of mocks?
There was a problem hiding this comment.
Was trying to avoid adding more wrapper function hence the lamda. I've updated the code to use mocks
| print("Error: Expected package manager dnf not found on this Azure Linux4 VM.") | ||
| return str() | ||
|
|
||
| code, out, version = self.__get_dnf_version() |
There was a problem hiding this comment.
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()
| if "dnf --version" in cmd: | ||
| if usecase == "wrong_version": | ||
| return 0, "dnf version 4.14.0" | ||
| if usecase == "version_command_failure": |
There was a problem hiding this comment.
This should be a switch case or an if else block with a single return at the end of the function
|
|
||
| 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') |
There was a problem hiding this comment.
Why a test for mocked functions?
There was a problem hiding this comment.
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
d416f37
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/core/src/package_managers/Dnf5PackageManager.py:732
- The file ends with multiple trailing blank lines (and one appears to contain whitespace). This creates noisy diffs and can trigger style/lint checks. Trim the extra whitespace so the file ends cleanly after
# endregion.
# endregion
| 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() | ||
| if code == 0 and version == '5': | ||
| return Constants.DNF5 | ||
| elif code == 0 and version != '5': |
| 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) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/core/tests/Test_EnvLayer.py:358
- In
test_mock_command_fallback_paths, these calls pass the string "wrong_version"/"version_command_failure" as theno_outputargument, so theusecaseparameter is never set. Because the command is alsodnf --v(which won't matchdnf --version), the test isn't actually exercising the wrong-version / command-failure branches of the mock and can give a false sense of coverage.
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)
src/core/src/bootstrap/EnvLayer.py:118
- The error printed when DNF5 detection fails doesn't include the
dnf --versionexit code/output, so it's still hard to diagnose why detection failed (command missing vs. permission error vs. unexpected output) without VM access. Since__get_dnf_version()already returns(code, out, version), includecode/outin the error message here as well.
else:
print("Error: Expected package manager dnf5 not found on this Azure Linux4 VM")
return str()
| # endregion | ||
|
|
||
|
|
88d5300 to
f948f05
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/core/tests/Test_EnvLayer.py:354
test_mock_command_fallback_pathscallsmock_run_command_for_dnf_version_command('dnf --v', "wrong_version")/("version_command_failure"), but that second argument is being bound tono_output(a boolean) rather thanusecase. This makes the test misleading and fragile (if the mock ever starts honoringno_output, this becomes an unintended truthy value).
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)
src/core/src/bootstrap/EnvLayer.py:118
- In the Azure Linux 4 branch, when
dnf --versionfails (non-zero exit code / empty output), the error printed does not include the return code or output. Including these details makes the failure diagnosable without VM access (e.g., command not found vs. permission vs. unexpected output).
else:
print("Error: Expected package manager dnf5 not found on this Azure Linux4 VM")
return str()
3.#343 (comment) (I already see 2 extra lines at the bottom )
Verified that all tests are running in both Python2 and Python3