[chores] Removed old, deprecated code#1190
Conversation
WalkthroughThis pull request removes four deprecated code elements scheduled for removal in version 1.2.0. The changes include: deletion of a wrapper function Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @openwisp_controller/config/controller/views.py:
- Around line 253-255: Add a clear "Breaking Changes" entry to CHANGES.rst for
version 1.2.0 stating that the agent status value "running" was removed from
config.STATUS (see allowed_status / required_params in controller.views) and
agents must now send "applied" instead of "running"; mention migration 0013
which updates DB records and explicitly warn deployed agents to update their
software to avoid validation failures.
In @setup.cfg:
- Around line 9-12: The exclude pattern currently set as "exclude =
./tests/*settings*.py" won’t match Flake8 paths (which are provided without
"./"); update the exclude entry to "exclude = tests/*settings*.py" so it
correctly excludes files like tests/openwisp2/settings.py, or alternatively
delete the exclude line and keep the existing per-file-ignores configuration if
you prefer relying solely on per-file-ignores.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_controller/config/base/config.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_controller.pysetup.cfg
💤 Files with no reviewable changes (3)
- openwisp_controller/config/tests/test_controller.py
- openwisp_controller/config/tasks.py
- openwisp_controller/config/base/config.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
🔇 Additional comments (1)
openwisp_controller/config/controller/views.py (1)
260-281: LGTM! Status handling implementation is correct.The removal of "running" status backward compatibility is clean. The implementation correctly:
- Validates status values against the allowed set
- Maintains the necessary "deactivating" → "deactivated" mapping for device lifecycle management
- Dynamically dispatches to appropriate
set_status_*methods- Handles error status with reason parameter
Note: The change is not documented in the Version 1.2.0 release notes. If this is a breaking change, it should be added to CHANGES.rst under a "Breaking Changes" section for Version 1.2.0 to inform users upgrading from earlier versions.
| exclude = ./tests/*settings*.py | ||
| max-line-length = 88 | ||
| per-file-ignores = | ||
| tests/openwisp2/settings.py:F401,F403 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read setup.cfg
fd -t f "setup.cfg" -x cat {}Repository: openwisp/openwisp-controller
Length of output: 366
🏁 Script executed:
# Check if the target settings file exists
fd -t f "settings.py" tests/Repository: openwisp/openwisp-controller
Length of output: 99
🏁 Script executed:
# Test if the exclude pattern ./tests/*settings*.py would match tests/openwisp2/settings.py
python3 << 'EOF'
import fnmatch
import os
# Test pattern matching
pattern = "./tests/*settings*.py"
test_path = "tests/openwisp2/settings.py"
test_path_with_dot = "./tests/openwisp2/settings.py"
# fnmatch uses Unix shell-style wildcards
print(f"Pattern: {pattern}")
print(f"Test path: {test_path}")
print(f"Match (without ./): {fnmatch.fnmatch(test_path, pattern)}")
print(f"Match (with ./): {fnmatch.fnmatch(test_path_with_dot, pattern)}")
# Also check with the pattern without ./
pattern_no_dot = "tests/*settings*.py"
print(f"\nPattern without ./: {pattern_no_dot}")
print(f"Match: {fnmatch.fnmatch(test_path, pattern_no_dot)}")
# Check if pattern would match specific file
test_path2 = "tests/openwisp2/settings.py"
print(f"\nDoes './tests/*settings*.py' match '{test_path2}'?")
print(f"Result: {fnmatch.fnmatch(test_path2, pattern)}")
print(f"Result (with ./): {fnmatch.fnmatch('./' + test_path2, pattern)}")
EOFRepository: openwisp/openwisp-controller
Length of output: 158
🏁 Script executed:
python3 << 'EOF'
import fnmatch
# Test pattern matching for flake8 exclude
pattern = "./tests/*settings*.py"
test_paths = [
"tests/openwisp2/settings.py",
"./tests/openwisp2/settings.py",
]
print("Pattern matching tests:")
for path in test_paths:
match = fnmatch.fnmatch(path, pattern)
print(f" fnmatch('{path}', '{pattern}') = {match}")
# The key issue: does ./ in pattern require ./ in path?
print("\nPattern analysis:")
print(f"Pattern has ./ prefix: {pattern.startswith('./')}")
print(f"This means it expects paths starting with './'")
print(f"But exclude in flake8 typically gets paths without './' prefix")
EOFRepository: openwisp/openwisp-controller
Length of output: 401
🌐 Web query:
flake8 exclude pattern ./ prefix behavior file path matching
💡 Result:
Short answer: Don't use a leading "./". Flake8 treats exclude entries as globs that it first tests against the base filename and then against the absolute path. A pattern starting with "./" is literal and generally won't match the absolute path Flake8 checks, so it will not reliably exclude the file. Use the directory/file path without the "./" (or use a glob like "*/subdir/file.py" or an absolute path) to get the expected exclusion. [1][2]
Sources:
- Flake8 options: --exclude matches filename first, then absolute path; patterns are globs (examples include "lib/python" expanding via os.path.abspath()). [1]
- Practical note / examples and behavior discussion on matching and how to target specific files (use project-relative paths or globs like "*/path/file.py"). [2]
References:
[1] flake8 documentation — Options (exclude/filename behavior).
[2] Stack Overflow — explanation of filename vs absolute-path matching and example patterns.
Fix the exclude pattern—it won't match the target file due to the ./ prefix.
The exclude pattern ./tests/*settings*.py is ineffective. Flake8 passes file paths without the leading ./ to the pattern matcher, so ./tests/*settings*.py won't match tests/openwisp2/settings.py. This is why per-file-ignores is needed as a workaround.
Either:
- Change
exclude = ./tests/*settings*.pytoexclude = tests/*settings*.py - Or remove the
excludeline entirely and rely solely onper-file-ignoresfor clarity
🤖 Prompt for AI Agents
In @setup.cfg around lines 9-12, The exclude pattern currently set as "exclude =
./tests/*settings*.py" won’t match Flake8 paths (which are provided without
"./"); update the exclude entry to "exclude = tests/*settings*.py" so it
correctly excludes files like tests/openwisp2/settings.py, or alternatively
delete the exclude line and keep the existing per-file-ignores configuration if
you prefer relying solely on per-file-ignores.
There was a problem hiding this comment.
I don't understand why this is needed? I think we can safely remove it.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the settings file exists and examine its content
fd -t f "settings.py" tests/ -x echo "File: {}" \; -x head -50 {}
Length of output: 1515
🏁 Script executed:
#!/bin/bash
# Run flake8 on the settings file to see what errors exist
cd tests/openwisp2 2>/dev/null && flake8 settings.py 2>&1 || echo "flake8 not available in sandbox"
# Alternative: check for import patterns that would trigger F401/F403
cat tests/openwisp2/settings.py 2>/dev/null | head -100
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check git history to see why this change was made
git log --oneline --all -p setup.cfg | grep -A5 -B5 "per-file-ignores" | head -30
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Get the full content of the settings file to check for star imports and unused imports
cat tests/openwisp2/settings.py
Length of output: 10045
🏁 Script executed:
#!/bin/bash
# Search for star imports that would trigger F403
grep -n "import \*" tests/openwisp2/settings.py
# Search for patterns that commonly appear in test settings
grep -n "from.*import" tests/openwisp2/settings.py | tail -20
Length of output: 75
@nemesifier You're right that we can take a cleaner approach here.
The reason this was added is because tests/openwisp2/settings.py has a star import at line 294:
from .local_settings import *This triggers F403 and F401 violations. However, the existing exclude pattern was already trying to handle this but doesn't work because of the ./ prefix issue that CodeRabbit flagged.
Instead of adding per-file-ignores, we should simply fix the existing exclude pattern by changing:
exclude = ./tests/*settings*.py
to:
exclude = tests/*settings*.py
That way the exclude pattern will work as originally intended, and we don't need the per-file-ignores addition at all. Would you prefer that approach?
17cdf14 to
31f4997
Compare
31f4997 to
9314b1c
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes deprecated code scheduled for removal in the 1.2.0 release, primarily backward compatibility features for the old "running" status value that was replaced by "applied" in earlier versions.
- Removed deprecated task
invalidate_device_checksum_view_cachefromtasks.py - Removed deprecated function
get_cached_checksum_args_rewritefrombase/config.py - Removed test for deprecated "running" status backward compatibility
- Updated flake8 configuration to add per-file-ignores for test settings
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.cfg | Added per-file-ignores configuration for F401/F403 in test settings |
| openwisp_controller/config/tests/test_controller.py | Removed test for deprecated "running" status backward compatibility |
| openwisp_controller/config/tasks.py | Removed deprecated invalidate_device_checksum_view_cache task |
| openwisp_controller/config/controller/views.py | Attempted to remove backward compatibility code for "running" status, but incomplete |
| openwisp_controller/config/base/config.py | Removed deprecated get_cached_checksum_args_rewrite function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nemesifier
left a comment
There was a problem hiding this comment.
Looks good, I have one comment below @Viscous106.
| exclude = ./tests/*settings*.py | ||
| max-line-length = 88 | ||
| per-file-ignores = | ||
| tests/openwisp2/settings.py:F401,F403 |
There was a problem hiding this comment.
I don't understand why this is needed? I think we can safely remove it.
I added this because my local linter was flagging F401/F403 errors in that file. However, I agree it's better to keep the config clean if it's not strictly necessary for the CI . I will remove it and push the update. Thanks! |
55806f5 to
8fd95ca
Compare
8fd95ca to
74e376a
Compare
|
Thanks! |
Supersedes #1182.
This PR removes the deprecated code marked for the 1.2.0 release.
Changes:
config.py.setup.cfgto correct linting configuration for test settings.Closes #1189