Fix Sanity and UT execution#308
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request modernizes the codebase by removing dependencies on the deprecated six library and migrating to native Python 3 syntax. The changes also update the CI/CD workflow to properly handle testing across different Ansible versions.
Key changes:
- Replaced all
iteritems()calls fromsixlibrary with native Python 3.items()method across multiple modules - Updated
safe_evalimport fromansible.module_utils.common.validationto useast.literal_evaldirectly - Enhanced GitHub Actions workflow to support testing with Ansible devel branch using Python 3.12 and added specific handling for coverage versions
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/modules/nios_range.py | Removed six.iteritems import and replaced usage with native .items() method |
| plugins/modules/nios_network.py | Removed six.iteritems import and replaced all usages with .items() method |
| plugins/modules/nios_host_record.py | Removed six.iteritems import and replaced usage with .items() method |
| plugins/modules/nios_fixed_address.py | Removed six.iteritems import and replaced all usages with .items() method |
| plugins/modules/nios_dtc_topology.py | Removed six.iteritems import and replaced usage with .items() method |
| plugins/modules/nios_dtc_monitor_snmp.py | Removed six.iteritems import and replaced usage with .items() method |
| plugins/module_utils/api.py | Removed six.iteritems import, replaced all usages with .items(), and updated safe_eval import/usage from validation module to ast.literal_eval |
| plugins/inventory/nios_inventory.py | Removed six.iteritems import and replaced usage with .items() method |
| .github/workflows/ansible-test.yml | Added Python 3.11 exclusion for devel, conditional utils.py handling for devel branch, coverage version pinning, and dynamic Python version selection for sanity tests |
Comments suppressed due to low confidence (2)
plugins/module_utils/api.py:903
- This assignment to 'txt' is unnecessary as it is redefined before this value is used.
This assignment to 'txt' is unnecessary as it is redefined before this value is used.
txt = result['old_text']
plugins/module_utils/api.py:969
- This assignment to 'txt' is unnecessary as it is redefined before this value is used.
This assignment to 'txt' is unnecessary as it is redefined before this value is used.
txt = result['old_text']
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| txt = result['new_text'] | ||
| except Exception: | ||
| raise TypeError('unable to evaluate string as dictionary') | ||
| txt = result['new_text'] |
There was a problem hiding this comment.
There is redundant code after the exception handling. Line 459 assigns txt = result['new_text'] after it was already assigned on line 456. This line should be removed as it's unreachable code that serves no purpose.
| txt = result['new_text'] |
| txt = result['old_text'] | ||
| old_text_exists = True |
There was a problem hiding this comment.
There is redundant code after the exception handling. Lines 907-908 assign txt = result['old_text'] and old_text_exists = True after they were already assigned on lines 903-904. These lines should be removed as they're unreachable code that serves no purpose.
| txt = result['old_text'] | |
| old_text_exists = True |
| txt = result['old_text'] | ||
| old_text_exists = True |
There was a problem hiding this comment.
There is redundant code after the exception handling. Lines 973-974 assign txt = result['old_text'] and old_text_exists = True after they were already assigned on lines 969-970. These lines should be removed as they're unreachable code that serves no purpose.
| txt = result['old_text'] | |
| old_text_exists = True |
| @@ -81,6 +83,14 @@ jobs: | |||
| git clone https://github.com/ansible/ansible.git -b ${{ matrix.ansible-version }} | |||
| if [ "${{ matrix.ansible-version }}" == "stable-2.16" ]; then cp -rf ansible/test/units/compat /home/runner/.ansible/collections/ansible_collections/infoblox/nios_modules/tests/unit/; fi | |||
| cp -rf ansible/test/units/modules/utils.py /home/runner/.ansible/collections/ansible_collections/infoblox/nios_modules/tests/unit/plugins/modules/ | |||
There was a problem hiding this comment.
The line 85 that copies utils.py is redundant because the same operation is performed within the conditional block on lines 91-92 when the ansible-version is NOT 'devel'. This will result in the file being copied twice unnecessarily for non-devel versions.
| cp -rf ansible/test/units/modules/utils.py /home/runner/.ansible/collections/ansible_collections/infoblox/nios_modules/tests/unit/plugins/modules/ |
| - ansible-version: devel | ||
| python-version: '3.10' | ||
| - ansible-version: devel | ||
| python-version: '3.11' |
There was a problem hiding this comment.
There is a trailing whitespace character after '3.11' that should be removed to maintain clean formatting and consistency.
| python-version: '3.11' | |
| python-version: '3.11' |
Unit and Sanity Tests are fixed