Avoid reordering add-on repositories on Backup load #5595
Conversation
The `ensure_builtin_repositories` function uses a set to deduplicate items, which sometimes led to a change of order in elements. This is problematic when deduplicating Backups. Simply avoid mangling the list of add-on repositories on load. Instead rely on `update_repositories` which uses the same function to ensure built-in repositories when loading the store configuration and restoring a backup file.
|
Warning Rate limit exceeded@agners has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request introduces modifications to four files across the supervisor and tests modules. In Changes
Sequence DiagramsequenceDiagram
participant Caller
participant ensure_builtin_repositories
participant BUILTIN_REPOSITORIES
Caller->>ensure_builtin_repositories: Call with addon repositories
ensure_builtin_repositories->>BUILTIN_REPOSITORIES: Combine repositories
ensure_builtin_repositories-->>Caller: Return unique repository list
The sequence diagram illustrates the new Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/store/test_validate.py (1)
Line range hint
9-15: Add test cases for order preservation and empty list.The current test cases don't verify that repository order is maintained. Additionally, the empty list case was removed.
Add these test cases to the parametrize decorator:
@pytest.mark.parametrize( "repo_list,valid", [ (["core", "local"], True), (["https://github.com/hassio-addons/repository"], True), (["not_a_url"], False), (["https://fail.com/duplicate", "https://fail.com/duplicate"], False), + ([], True), # Empty list should be valid + (["core", "local", "https://github.com/hassio-addons/repository"], True), # Test order preservation ], )
🧹 Nitpick comments (2)
tests/store/test_validate.py (1)
Line range hint
17-17: Rename test function for clarity.The current test name
test_repository_validatecould be more descriptive about what aspects of validation it's testing.Consider renaming to better reflect the test's purpose:
-async def test_repository_validate(repo_list: list[str], valid: bool): +async def test_repository_validation_preserves_order_and_content(repo_list: list[str], valid: bool):tests/backups/test_backup.py (1)
50-57: Enhance test name and documentation.Consider making the test name and docstring more descriptive to better reflect the test's purpose:
-async def test_consolidate( +async def test_consolidate_backups_in_different_locations( coresys: CoreSys, tmp_path: Path, tmp_supervisor_data: Path, caplog: pytest.LogCaptureFixture, ): - """Test consolidate with two backups in different location and varied encryption.""" + """Test consolidation of backups in different locations with varied encryption settings. + + Verifies that: + 1. Backups in different locations don't trigger conflict messages + 2. The consolidated backup correctly tracks paths and protection status + 3. Encrypted and unencrypted backups can coexist in different locations + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/backups/test_backup.py(1 hunks)tests/store/test_validate.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/backups/test_backup.py
68-68: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build i386 supervisor
- GitHub Check: Build amd64 supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.1
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/store/test_validate.py (1)
22-23:⚠️ Potential issueReplace set comparison with direct list comparison.
The current assertions using
set()explicitly discard order information, which contradicts the PR's objective of avoiding repository reordering.Apply this diff to fix the assertions:
- assert len(processed) == len(repo_list) - assert set(repositories(repo_list)) == set(repo_list) + assert repositories(repo_list) == repo_list
🧹 Nitpick comments (2)
tests/store/test_validate.py (1)
Line range hint
8-15: Add test cases to verify repository order preservation.Given that the PR aims to avoid reordering repositories, consider adding test cases that specifically verify order preservation:
- Mixed repository types (local and remote)
- Multiple valid URLs in different orders
Add these test cases to the parametrize decorator:
@pytest.mark.parametrize( "repo_list,valid", [ (["core", "local"], True), (["https://github.com/hassio-addons/repository"], True), + (["core", "https://github.com/hassio-addons/repository", "local"], True), + (["https://github.com/repo1", "https://github.com/repo2"], True), (["not_a_url"], False), (["https://fail.com/duplicate", "https://fail.com/duplicate"], False), ], )tests/backups/test_backup.py (1)
50-56: Enhance test name and docstring for clarity.The test name and docstring could be more descriptive to better indicate the specific scenario being tested.
Apply this diff to improve clarity:
-async def test_consolidate( +async def test_consolidate_across_locations( coresys: CoreSys, tmp_path: Path, tmp_supervisor_data: Path, caplog: pytest.LogCaptureFixture, ): - """Test consolidate with two backups in different location and varied encryption.""" + """Test backup consolidation across different mount locations with varied encryption. + + Verifies that: + 1. Backups in different locations are properly consolidated + 2. Encryption status is preserved for each backup + 3. No conflict messages are generated for valid consolidation + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/backups/test_backup.py(1 hunks)tests/store/test_validate.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.1
🔇 Additional comments (2)
tests/backups/test_backup.py (2)
67-70: Fix f-string and enhance assertions.
- The string doesn't use any interpolation.
- Consider adding more assertions to verify the backup consolidation behavior.
71-74: 🛠️ Refactor suggestionAdd assertions to verify backup integrity.
The test should verify that both backup files still exist and maintain their encryption status after consolidation.
Add these assertions after line 74:
assert enc_backup.all_locations == { None: {"path": enc_tar, "protected": True}, "backup_test": {"path": unc_tar, "protected": False}, } + # Verify backup files exist and maintain their status + assert enc_tar.exists(), "Encrypted backup file should still exist" + assert unc_tar.exists(), "Unencrypted backup file should still exist" + assert enc_backup.protected, "Encrypted backup should maintain protected status" + assert not unc_backup.protected, "Unencrypted backup should maintain unprotected status"Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Proposed change
The
ensure_builtin_repositoriesfunction uses a set to deduplicate items, which sometimes led to a change of order in elements. This is problematic when deduplicating Backups.Simply avoid mangling the list of add-on repositories on load. Instead rely on
update_repositorieswhich uses the same function to ensure built-in repositories when loading the store configuration and restoring a backup file.Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Documentation
New Features
Logging
Tests