-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
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.
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
Import Influxdb integration level configuration into ConfigEntry #134463
base: dev
Are you sure you want to change the base?
Conversation
Hey there @mdegat01, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
2c5fd6b
to
057e22a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @RobBie1221, I've seen your name around! If you feel like it, feel free to drop me a message on Discord :)
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
b9d52c6
to
c61519e
Compare
I addressed review comments. I also made a change to creating the config entry, I split the data and options (data that we later want to change via option flow is now stored in the options part of the config entry). |
44e503e
to
9584ba0
Compare
9584ba0
to
d85e04c
Compare
if conf.get(CONF_SSL_CA_CERT) is not None and conf[CONF_VERIFY_SSL]: | ||
kwargs[CONF_VERIFY_SSL] = conf[CONF_SSL_CA_CERT] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we change this btw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During import a dict is created with all keys. When the value is missing in yaml, the value is set to None.
Previously it was sufficient to check if a key is present, now we check for None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case
if conf.get(CONF_SSL_CA_CERT) is not None and conf[CONF_VERIFY_SSL]: | |
kwargs[CONF_VERIFY_SSL] = conf[CONF_SSL_CA_CERT] | |
if (cert := conf.get(CONF_SSL_CA_CERT)) is not None and conf[CONF_VERIFY_SSL]: | |
kwargs[CONF_VERIFY_SSL] = cert |
return None | ||
|
||
|
||
class InfluxDBConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class InfluxDBConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): | |
class InfluxDBConfigFlow(ConfigFlow, domain=DOMAIN): |
if data is not None and options is not None: | ||
changed = self.hass.config_entries.async_update_entry( | ||
entry, | ||
data={**entry.data, **data}, | ||
options={**entry.options, **options}, | ||
) | ||
if changed and entry.state in ( | ||
config_entries.ConfigEntryState.LOADED, | ||
config_entries.ConfigEntryState.SETUP_RETRY, | ||
): | ||
self.hass.async_create_task( | ||
self.hass.config_entries.async_reload(entry.entry_id) | ||
) | ||
raise AbortFlow("already_configured") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't sound like aborting, rather like updating and aborting. Ideally we don't do that anymore and have users use reconfigure for this (unless I am missing something why we do this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that with the amount of options, we need quite a complex option flow for reconfigure. With this behavior, it always updates from the yaml.
If we don’t want this intermediate behavior, either this PR increases in size (which I wouldn’t do) or we don’t push to dev but an intermediate branch to push all the changes incrementally to have a full config flow and push that in the end to dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think like we spoke before, there are also options that are nearly impossible to properly get into a config flow, so I think a good first step that would be mergable would be to just setup the connection via the UI and do the rest via YAML (+ maybe some simple options).
I kinda want to keep the import and user step in the same PR, mostly because the way you do one, affects the other (schema, required data, etc). So if we would merge this as is, and we have to change stuff, we have a harder time in having a clear overview in why we did what we did.
And nevertheless, I am committed on getting this through, so don't be scared to be hit with a "this is too big" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let’s do the import and user step together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and to continue on reconfiguring, I think that is something that we can do quite fast after implementing this one, so the options are still changeable. Everything not required for the connection (the optional things) can be set using the options flow. I think it would be nice to be in here, but I also don't have the full scope of influxdb in my mind, so if that is indeed one beast, I think adding reconfigure and options for a followup with intermediary branch is nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add user step here and only use yaml for options. It may be a bit confusing that we only partially use yaml for reconfiguring (basic properties for connection are then ignored). We can make a clear description for that.
Option flow can be done later then where we can decide which options to transfer to UI and which to keep in yaml.
"config": { | ||
"step": { | ||
"import": { | ||
"data": {}, | ||
"title": "Import configuration" | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember we talked about this one before, let's also add a user step so users can also actually set up the integration via the config flow
Proposed change
Import integration level configuration for influxdb integration into ConfigEntry. Implement load/unload entry functions. Platform level setup will be dealt with in a different PR, as well as full config/option flow.
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.To help with the load of incoming pull requests: