Add BaseConfigUpdateEvent and handle base configuration updates #747
Conversation
WalkthroughThe recent updates focus on enhancing configuration management within the LedFX application. Key improvements include the introduction of mechanisms to dynamically update configurations without necessitating a restart for certain core settings. A new event class, Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- ledfx/api/config.py (3 hunks)
- ledfx/config.py (1 hunks)
- ledfx/core.py (5 hunks)
- ledfx/events.py (2 hunks)
Additional comments: 6
ledfx/events.py (1)
- 170-178: The implementation of
BaseConfigUpdateEventis concise and follows the established pattern for event classes in this file. It correctly inherits from theEventclass and initializes with theBASE_CONFIG_UPDATEevent type. This addition is crucial for supporting dynamic configuration updates as outlined in the PR objectives.ledfx/api/config.py (1)
- 220-273: The
update_configmethod is well-structured and effectively updates various parts of the configuration, including audio, WLED preferences, melbanks, and core configuration. It also handles the special case for nested dictionaries inwled_preferences. The method concludes by firing aBaseConfigUpdateEventif there are changes in the core configuration. This method is crucial for applying configuration updates dynamically. Ensure that changes towled_preferencesare thoroughly tested, especially the handling of nested dictionaries, to avoid potential issues with partial updates.ledfx/core.py (2)
- 123-143: The addition of
handle_base_configuration_updatemethod and its integration with theBASE_CONFIG_UPDATEevent listener is a significant improvement. This method ensures that visualisation configuration updates are dynamically applied by re-setting up visualisation events. This aligns with the PR's objectives to enhance the application's adaptability and user interface in response to configuration changes. Ensure that the condition in line 141 is correctly identifying changes to visualization-related configuration keys.- 232-244: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [189-241]
The modifications to
setup_visualisation_eventsto remove existing listeners before setting up new ones are crucial for avoiding duplicate listeners and potential performance issues. This change ensures that visualisation updates are handled efficiently, especially after configuration changes that affect visualisation settings. It's a good practice to ensure that event listeners are managed properly to prevent memory leaks and ensure that updates are processed as expected.ledfx/config.py (2)
- 38-46: The addition of
CORE_CONFIG_KEYS_NO_RESTARTis aligned with the PR's objective to handle certain configuration changes dynamically without requiring a restart. This list should be carefully reviewed and updated as the application evolves to ensure it includes all configuration keys that can be updated on the fly. It's also important to ensure that the application logic elsewhere properly checks this list when handling configuration updates.- 48-51: The
VISUALISATION_CONFIG_KEYSlist is introduced to manage visualization-related configuration updates dynamically. This is a good approach to segregate visualization-specific settings, which can enhance the user experience by allowing for immediate updates to visualization parameters without needing a restart. Ensure that the visualization event listeners and the related logic are correctly updated to check against this list when handling configuration changes.
|
|
||
| try: | ||
| config = await request.json() | ||
| except JSONDecodeError: | ||
| return await self.json_decode_error() | ||
| except Exception as e: | ||
| return await self.generic_error(str(e)) | ||
|
|
||
| audio_config = validate_and_trim_config( | ||
| config.pop("audio", {}), | ||
| AudioInputSource.AUDIO_CONFIG_SCHEMA.fget(), | ||
| "audio", | ||
| ) | ||
| wled_config = validate_and_trim_config( | ||
| config.pop("wled_preferences", {}), | ||
| WLED_CONFIG_SCHEMA, | ||
| "wled_preferences", | ||
| ) | ||
| melbanks_config = validate_and_trim_config( | ||
| config.pop("melbanks", {}), Melbanks.CONFIG_SCHEMA, "melbanks" | ||
| try: | ||
| self.update_config(config) | ||
| self._ledfx.events.fire_event(BaseConfigUpdateEvent(config)) | ||
| save_config( | ||
| config=self._ledfx.config, config_dir=self._ledfx.config_dir | ||
| ) | ||
| core_config = validate_and_trim_config( | ||
| config, CORE_CONFIG_SCHEMA, "core" | ||
| need_restart = self.check_need_restart(config) | ||
| if need_restart: | ||
| # Ugly - return success to frontend before restarting | ||
| try: | ||
| return await self.request_success( | ||
| type="success", | ||
| message="LedFx is restarting to apply the new configuration", | ||
| ) | ||
| finally: | ||
| self._ledfx.loop.call_soon_threadsafe(self._ledfx.stop, 4) | ||
| return await self.request_success( | ||
| type="success", message="Configuration Updated" | ||
| ) | ||
| except (KeyError, vol.MultipleInvalid) as msg: | ||
| error_message = f"Error updating config: {msg}" | ||
| _LOGGER.warning(error_message) | ||
| return await self.invalid_request(error_message) | ||
| except Exception as e: |
There was a problem hiding this comment.
The modifications to the put method to handle configuration updates and potential restarts are well-implemented. The method now correctly updates the configuration, fires a BaseConfigUpdateEvent, and checks if a restart is necessary based on the updated configuration. This aligns with the PR's objectives to enhance configuration management and minimize disruptions caused by restarts. However, consider adding more detailed logging for each step to improve traceability and debugging.
+ _LOGGER.info("Updating LedFx configuration.")
+ _LOGGER.info("Firing BaseConfigUpdateEvent.")
+ _LOGGER.info("Checking if restart is needed.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| try: | |
| config = await request.json() | |
| except JSONDecodeError: | |
| return await self.json_decode_error() | |
| except Exception as e: | |
| return await self.generic_error(str(e)) | |
| audio_config = validate_and_trim_config( | |
| config.pop("audio", {}), | |
| AudioInputSource.AUDIO_CONFIG_SCHEMA.fget(), | |
| "audio", | |
| ) | |
| wled_config = validate_and_trim_config( | |
| config.pop("wled_preferences", {}), | |
| WLED_CONFIG_SCHEMA, | |
| "wled_preferences", | |
| ) | |
| melbanks_config = validate_and_trim_config( | |
| config.pop("melbanks", {}), Melbanks.CONFIG_SCHEMA, "melbanks" | |
| try: | |
| self.update_config(config) | |
| self._ledfx.events.fire_event(BaseConfigUpdateEvent(config)) | |
| save_config( | |
| config=self._ledfx.config, config_dir=self._ledfx.config_dir | |
| ) | |
| core_config = validate_and_trim_config( | |
| config, CORE_CONFIG_SCHEMA, "core" | |
| need_restart = self.check_need_restart(config) | |
| if need_restart: | |
| # Ugly - return success to frontend before restarting | |
| try: | |
| return await self.request_success( | |
| type="success", | |
| message="LedFx is restarting to apply the new configuration", | |
| ) | |
| finally: | |
| self._ledfx.loop.call_soon_threadsafe(self._ledfx.stop, 4) | |
| return await self.request_success( | |
| type="success", message="Configuration Updated" | |
| ) | |
| except (KeyError, vol.MultipleInvalid) as msg: | |
| error_message = f"Error updating config: {msg}" | |
| _LOGGER.warning(error_message) | |
| return await self.invalid_request(error_message) | |
| except Exception as e: | |
| try: | |
| config = await request.json() | |
| except JSONDecodeError: | |
| return await self.json_decode_error() | |
| except Exception as e: | |
| return await self.generic_error(str(e)) | |
| try: | |
| _LOGGER.info("Updating LedFx configuration.") | |
| self.update_config(config) | |
| _LOGGER.info("Firing BaseConfigUpdateEvent.") | |
| self._ledfx.events.fire_event(BaseConfigUpdateEvent(config)) | |
| save_config( | |
| config=self._ledfx.config, config_dir=self._ledfx.config_dir | |
| ) | |
| _LOGGER.info("Checking if restart is needed.") | |
| need_restart = self.check_need_restart(config) | |
| if need_restart: | |
| # Ugly - return success to frontend before restarting | |
| try: | |
| return await self.request_success( | |
| type="success", | |
| message="LedFx is restarting to apply the new configuration", | |
| ) | |
| finally: | |
| self._ledfx.loop.call_soon_threadsafe(self._ledfx.stop, 4) | |
| return await self.request_success( | |
| type="success", message="Configuration Updated" | |
| ) | |
| except (KeyError, vol.MultipleInvalid) as msg: | |
| error_message = f"Error updating config: {msg}" | |
| _LOGGER.warning(error_message) | |
| return await self.invalid_request(error_message) | |
| except Exception as e: |
| def check_need_restart(self, config): | ||
| """ | ||
| Checks if a restart is needed based on the provided configuration. | ||
|
|
||
| Args: | ||
| config (dict): The configuration to be checked. | ||
|
|
||
| Returns: | ||
| bool: True if a restart is needed, False otherwise. | ||
| """ | ||
| core_config = validate_and_trim_config( | ||
| config, CORE_CONFIG_SCHEMA, "core" | ||
| ) | ||
|
|
||
| # If core_config is empty, no restart is needed | ||
| if not core_config: | ||
| return False | ||
|
|
||
| need_restart = True | ||
| if any(key in core_config for key in CORE_CONFIG_KEYS_NO_RESTART): | ||
| need_restart = False | ||
|
|
||
| return need_restart |
There was a problem hiding this comment.
The check_need_restart method correctly assesses whether a restart is necessary based on the provided configuration. It uses CORE_CONFIG_KEYS_NO_RESTART to determine if any of the updated keys allow for dynamic updates without a restart. This logic is essential for minimizing disruptions. However, consider enhancing this method to log which specific configuration changes are triggering a restart, as this could aid in debugging and user understanding.
+ _LOGGER.debug(f"Checking need for restart with updated config keys: {list(config.keys())}")
+ if need_restart:
+ _LOGGER.info("A restart is required due to changes in the core configuration.")
+ else:
+ _LOGGER.info("No restart is required for the applied configuration changes.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def check_need_restart(self, config): | |
| """ | |
| Checks if a restart is needed based on the provided configuration. | |
| Args: | |
| config (dict): The configuration to be checked. | |
| Returns: | |
| bool: True if a restart is needed, False otherwise. | |
| """ | |
| core_config = validate_and_trim_config( | |
| config, CORE_CONFIG_SCHEMA, "core" | |
| ) | |
| # If core_config is empty, no restart is needed | |
| if not core_config: | |
| return False | |
| need_restart = True | |
| if any(key in core_config for key in CORE_CONFIG_KEYS_NO_RESTART): | |
| need_restart = False | |
| return need_restart | |
| def check_need_restart(self, config): | |
| """ | |
| Checks if a restart is needed based on the provided configuration. | |
| Args: | |
| config (dict): The configuration to be checked. | |
| Returns: | |
| bool: True if a restart is needed, False otherwise. | |
| """ | |
| core_config = validate_and_trim_config( | |
| config, CORE_CONFIG_SCHEMA, "core" | |
| ) | |
| # If core_config is empty, no restart is needed | |
| if not core_config: | |
| return False | |
| need_restart = True | |
| if any(key in core_config for key in CORE_CONFIG_KEYS_NO_RESTART): | |
| need_restart = False | |
| _LOGGER.debug(f"Checking need for restart with updated config keys: {list(config.keys())}") | |
| if need_restart: | |
| _LOGGER.info("A restart is required due to changes in the core configuration.") | |
| else: | |
| _LOGGER.info("No restart is required for the applied configuration changes.") | |
| return need_restart |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This pull request adds a new event called BaseConfigUpdateEvent and implements the handling of base configuration updates.
It also includes the necessary changes to setup visualisation events when the base configuration is updated without restarting.
Adds snackbars when base config is updated.
Summary by CodeRabbit