Update unique-config-entry rule #2510
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe documentation for handling unique configuration entries in integrations has been updated to provide clearer guidance on preventing duplicate device or service configurations. The document now offers more detailed explanations about why and how to block duplicate configurations, including two specific methods for ensuring uniqueness: using unique identifiers and checking specific configuration fields. Changes
Sequence DiagramsequenceDiagram
participant Integration
participant ConfigFlow
participant ExistingConfigs
Integration->>ConfigFlow: Attempt to create new configuration
ConfigFlow->>ExistingConfigs: Check for existing unique ID
alt Unique ID already exists
ConfigFlow-->>Integration: Abort configuration flow
else Unique ID is new
ConfigFlow->>ExistingConfigs: Check specific config fields
alt Matching config fields found
ConfigFlow-->>Integration: Abort configuration flow
else No matching config found
ConfigFlow->>ExistingConfigs: Create new configuration
end
end
The sequence diagram illustrates the process of checking for unique configuration entries, showing how an integration would validate and prevent duplicate configurations through checks on unique identifiers and specific configuration fields. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (1)
docs/core/integration-quality-scale/rules/unique-config-entry.md (1)
20-21: LGTM! Clear improvement to the documentation.The added text effectively clarifies that modifications to existing configurations should be handled through reconfiguration flows rather than the initial configuration flow. This helps prevent confusion about which fields get updated during reconfiguration.
Consider adding an example implementation of a reconfiguration flow to complement the existing examples, demonstrating how to properly handle updates to an existing configuration entry. This would provide a complete picture of the recommended approach.
Example structure to consider adding:
### Reconfiguration flow When a user needs to modify an existing configuration (e.g., update credentials or connection details), use a reconfiguration flow instead of the initial configuration flow. This ensures clarity about which fields are being updated. Example implementation of a reconfiguration flow: [code example here]
docs/core/integration-quality-scale/rules/unique-config-entry.md
Outdated
Show resolved
Hide resolved
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Abílio Costa <[email protected]>
| @@ -17,6 +17,8 @@ This can lead to duplicated devices and entities with unique identifiers collidi | |||
| Any discovery flow must also ensure that a config entry is uniquely identifiable, as otherwise, it would discover devices already set up. | |||
|
|
|||
| To prevent this, we need to ensure that the user can only set up a device or service once. | |||
| Trying to set up a device or service that is already set up should be prevented by the integration. | |||
| If an update to the configuration is required, it should be handled by the reconfiguration flow instead, as it allows us to be more consistent with what gets updated. | |||
There was a problem hiding this comment.
Should this discuss reauth as the only exception to this?
There was a problem hiding this comment.
Hmm, the goal is to disallow starting a config flow manually and then trying to setup the same device and automatically update things like the host and port. Not necessarily to only allow the reconfigure flow to update the config. (like, we still want to update the entry via code if needed, so I don't want to slam that door closed)
There was a problem hiding this comment.
True, I don't intend to suggest that reauth should be updating configuration outside of authentication credentials. I was thrown off by the phrase "update to the configuration" which sounds broad, but i understand you're implicitly allowing an exception for authentication parts of the configuration.
There was a problem hiding this comment.
But you are a native English speaker, so if you have improvements to convey this better, please let me know 🙏🏻
| Trying to set up a device or service that is already set up should be prevented by the integration. | ||
| If an update to the configuration is required, it should be handled by the reconfiguration flow instead, as it allows us to be more consistent with what gets updated. |
There was a problem hiding this comment.
Yes, though I also learned that means my english is sloppy and less precise :)
That said, this is how I understand it:
| Trying to set up a device or service that is already set up should be prevented by the integration. | |
| If an update to the configuration is required, it should be handled by the reconfiguration flow instead, as it allows us to be more consistent with what gets updated. | |
| Updates to an existing device or service configuration are only allowed through the reconfiguration flow. Updates to authentication credentials are allowed in a reauthentication flow. Otherwise, trying to set up a device or service that is already set up should be prevented by the integration. |
| @@ -17,6 +17,8 @@ This can lead to duplicated devices and entities with unique identifiers collidi | |||
| Any discovery flow must also ensure that a config entry is uniquely identifiable, as otherwise, it would discover devices already set up. | |||
|
|
|||
| To prevent this, we need to ensure that the user can only set up a device or service once. | |||
| Trying to set up a device or service that is already set up should be prevented by the integration. | |||
| If an update to the configuration is required, it should be handled by the reconfiguration flow instead, as it allows us to be more consistent with what gets updated. | |||
There was a problem hiding this comment.
How about this as an alternative:
| If an update to the configuration is required, it should be handled by the reconfiguration flow instead, as it allows us to be more consistent with what gets updated. | |
| Configuration flows initiated manually by the user to set up a new entry should not modify or update existing configuration entries. For these purposes, we have our discovery, options, and reconfiguration flows available instead. |
There was a problem hiding this comment.
So to set the terms straight, what do we use Configuration flows for? because all flows like discovery, options, reauth and reconfigure flow are in theory configuration flows right?
Because with that in mind, the reconfigure flow is also a configuration flow initiated by the user
There was a problem hiding this comment.
Yes: Hence, with the intention to set up a new entry being mentioned there. This will also fit the bill for anything that comes down road regarding nuggets, that also cause new entries.
Basically, we want to prevent existing entries from being modified while the user had the intention of creating a new entry.
Proposed change
The current unique-config-entry rule does not cover that a config flow started by the user should not be used to update data of an existing config entry. Instead, devs should implement and users should use the reconfiguration flow. This leaves for a more consistent user experience (since does setting up the device again update every field? Only the Host? Only the password? An end user can only guess).
So let's update the rule to mention that we don't want to re-use the config flow to change existing entry data.
Type of change
Additional information
Summary by CodeRabbit