Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update unique-config-entry rule #2510

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Update unique-config-entry rule #2510

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8217581
Update unique-config-entry rule
joostlek Dec 19, 2024
9abb060
Update docs/core/integration-quality-scale/rules/unique-config-entry.md
joostlek Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/core/integration-quality-scale/rules/unique-config-entry.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this discuss reauth as the only exception to this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you are a native English speaker, so if you have improvements to convey this better, please let me know 🙏🏻

Comment on lines +20 to +21
Copy link
Contributor

@allenporter allenporter Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, though I also learned that means my english is sloppy and less precise :)

That said, this is how I understand it:

Suggested change
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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this as an alternative:

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@frenck frenck Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


## Example implementation

Expand Down