Delay initial version fetch until there is connectivity #5603
Conversation
📝 WalkthroughWalkthroughThe changes update the handling of network connectivity. In the production code, the type of a connectivity variable is changed from a boolean to an integer, and a new asynchronous connectivity check method is added in the updater. The updater’s load method now delays fetching version data until a valid network connection is detected. Additionally, a new enumeration value for connectivity change events is introduced. In the test code, the network manager mock is updated with a flag for connectivity checking, and a new test case is added to validate the delayed fetching behavior based on connectivity. Changes
Sequence Diagram(s)sequenceDiagram
participant U as Updater
participant NM as NetworkManager
participant DBus as DBus Event Source
participant WS as WebSession
U->>NM: load() checks current connectivity state
alt Connectivity check enabled and not fully connected
U->>DBus: Register _check_connectivity listener
Note over U: Wait for connectivity change events
else Fully connected or check disabled
U->>WS: Fetch version data immediately
end
DBus->>U: Trigger _check_connectivity event on change
alt Connectivity becomes full
U->>WS: Fetch version data
U->>DBus: Unregister connectivity listener
else
Note over U: Continue listening for connectivity updates
end
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (6)`*/**(html|markdown|md)`: - For instructional content in doc...
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
`*/**(html|markdown|md)`: - Be brief in your replies and don...
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (10)
✨ Finishing Touches
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: 0
🧹 Nitpick comments (2)
supervisor/updater.py (1)
200-220: Consider adding error handling for the fetch operation.While the implementation correctly handles connectivity state changes, the fetch operation in a suppressed context might hide important errors.
Consider this alternative implementation:
async def _check_connectivity( self, interface: str, changed: dict[str, Any], invalidated: list[str] ): """Check if connectivity is true and fetch data.""" if interface != DBUS_IFACE_NM: return connectivity_check: bool | None = changed.get(DBUS_ATTR_CONNECTION_ENABLED) connectivity: bool | None = changed.get(DBUS_ATTR_CONNECTIVITY) # If there's no connectivity checks, stop waiting for connection # Else when host says we're online, attempt to fetch version data and disable listener if ( not connectivity_check or connectivity == ConnectivityState.CONNECTIVITY_FULL ): self.sys_dbus.network.dbus.properties.off_properties_changed( self._check_connectivity ) - with suppress(UpdaterError): - await self.fetch_data() + try: + await self.fetch_data() + except UpdaterError as err: + _LOGGER.warning("Failed to fetch version data: %s", err)tests/test_updater.py (1)
81-114: Consider adding more test cases for comprehensive coverage.While the current test cases cover the basic scenarios, consider adding these additional cases:
- Connectivity transitions from NONE to LIMITED
- Connectivity transitions from LIMITED to FULL
- Multiple connectivity state changes
Add these test cases to the parameterization:
@pytest.mark.parametrize( ("connectivity", "check_enabled"), [ (ConnectivityState.CONNECTIVITY_FULL, True), (ConnectivityState.CONNECTIVITY_NONE, False), + (ConnectivityState.CONNECTIVITY_LIMITED, True), + (ConnectivityState.CONNECTIVITY_PORTAL, True), ], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supervisor/host/network.py(1 hunks)supervisor/updater.py(4 hunks)tests/dbus_service_mocks/network_manager.py(2 hunks)tests/test_updater.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in doc...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
supervisor/updater.pytests/test_updater.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
supervisor/updater.pytests/test_updater.py
`*/**(html|markdown|md)`: - Be brief in your replies and don...
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
supervisor/updater.pytests/test_updater.py
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
supervisor/updater.pytests/test_updater.py
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
supervisor/updater.pytests/test_updater.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
supervisor/updater.pytests/test_updater.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.13.1
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (5)
supervisor/updater.py (1)
60-68: LGTM! Delay version fetch until connectivity is established.The implementation correctly delays the initial version fetch when there's no network connectivity, setting up a listener for connectivity changes.
supervisor/host/network.py (1)
151-151: LGTM! Type change aligns with ConnectivityState enum.The type change from
bool | Nonetoint | Nonecorrectly matches the DBUS interface's connectivity state values.tests/dbus_service_mocks/network_manager.py (1)
25-25: LGTM! Mock correctly implements connectivity check control.The
connectivity_check_enabledattribute allows tests to control the connectivity check state.tests/test_updater.py (2)
95-103: LGTM! Test correctly verifies delayed fetch behavior.The test properly sets up the initial state and verifies that no fetch occurs without connectivity.
105-114: LGTM! Test correctly verifies fetch on connectivity change.The test properly verifies that version information is fetched when connectivity is established or checks are disabled.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_updater.py (2)
106-110: Add delay between network change and ping to prevent race conditions.The test might be susceptible to race conditions as it immediately pings after emitting the property change.
Add a small delay to ensure the property change is processed:
network_manager_service.emit_properties_changed( {"Connectivity": connectivity.value, "ConnectivityCheckEnabled": check_enabled} ) + await asyncio.sleep(0.1) # Allow time for property change to be processed await network_manager_service.ping()
111-114: Use URL_TEST constant instead of hardcoded URL.The test uses a hardcoded URL for assertion while the same URL is defined as URL_TEST constant at the top of the file.
Apply this diff to use the constant:
assert ( coresys.websession.get.call_args[0][0] - == "https://version.home-assistant.io/stable.json" + == URL_TEST )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_updater.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in doc...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
tests/test_updater.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
tests/test_updater.py
`*/**(html|markdown|md)`: - Be brief in your replies and don...
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
tests/test_updater.py
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
tests/test_updater.py
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
tests/test_updater.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
tests/test_updater.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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
🔇 Additional comments (1)
tests/test_updater.py (1)
3-3: LGTM! Import changes are appropriate.The new imports support the connectivity testing requirements.
Also applies to: 9-9, 11-13
mdegat01
changed the title
| self.sys_bus.fire_event(BusEvent.SUPERVISOR_JOB_START, job.uuid) | ||
| self.sys_bus.fire_event(BusEvent.SUPERVISOR_JOB_START, job) | ||
| if value is True: | ||
| self.sys_bus.fire_event(BusEvent.SUPERVISOR_JOB_END, job.uuid) | ||
| self.sys_bus.fire_event(BusEvent.SUPERVISOR_JOB_END, job) |
There was a problem hiding this comment.
I realize this feels unrelated. As it turns out, trying to use these in test made me realize they are entirely useless. The listener only receives a uuid and it can't actually count on the job still being around to look up more info about it, so there's really no way to reliably know what job started/ended. Passing the actual job in the event makes these useful (and it was needed in the test)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/jobs/test_job_decorator.py (1)
1199-1206: Update event listener parameter types for consistency.The event listeners in
test_job_scheduled_atstill usestrparameter type while similar listeners intest_job_scheduled_delayhave been updated to useSupervisorJob.Apply this diff to maintain consistency:
-async def start_listener(job_id: str): +async def start_listener(evt_job: SupervisorJob): nonlocal started - started = started or job_id == job.uuid + started = started or evt_job.uuid == job.uuid -async def end_listener(job_id: str): +async def end_listener(evt_job: SupervisorJob): nonlocal ended - ended = ended or job_id == job.uuid + ended = ended or evt_job.uuid == job.uuid
🧹 Nitpick comments (1)
tests/test_updater.py (1)
110-110: Add cleanup for event listener registration.The event listener is registered but never unregistered, which could affect other tests. Consider using a fixture or adding cleanup code.
+ try: coresys.bus.register_event(BusEvent.SUPERVISOR_JOB_START, find_fetch_data_job_start) + # ... rest of the test ... + finally: + coresys.bus.unregister_event(BusEvent.SUPERVISOR_JOB_START, find_fetch_data_job_start)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
supervisor/jobs/__init__.py(1 hunks)tests/jobs/test_job_decorator.py(1 hunks)tests/test_updater.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in doc...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
tests/test_updater.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
tests/test_updater.py
`*/**(html|markdown|md)`: - Be brief in your replies and don...
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
tests/test_updater.py
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
tests/test_updater.py
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
tests/test_updater.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
tests/test_updater.py
⏰ 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 (4)
tests/test_updater.py (2)
3-18: LGTM!The imports are well-organized and provide the necessary modules for testing network connectivity functionality.
86-134: LGTM! Well-structured test implementation.The test effectively validates the delayed version fetch behavior using events instead of sleep loops. The staged testing approach (no connectivity → restored connectivity) with clear comments makes the test intent obvious.
supervisor/jobs/__init__.py (1)
216-218: LGTM! Enhanced event payload with full job context.The change to pass the entire
SupervisorJobobject instead of just the UUID provides more context to event listeners, which is a better practice for event-driven systems.tests/jobs/test_job_decorator.py (1)
1147-1154: LGTM! Updated event listener parameter types.The change to use
SupervisorJobinstead ofstrfor event listener parameters improves type safety and aligns with the changes in the event firing logic.
mdegat01
force-pushed
the
connectivity-for-version-fetch
branch
from
a6c9644 to
282d620
Compare
Proposed change
Delay initial fetch of version information if there is no network connectivity on the host until either it has been re-established or the connectivity check has been disabled. This way users that turn on their systems before plugging in the ethernet cable or setting up wifi don't have to reboot or force a reload via the CLI to get started.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes
Tests