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.

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

Draft
wants to merge 27 commits into
base: dev
Choose a base branch
from

Conversation

RobBie1221
Copy link
Contributor

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

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@home-assistant
Copy link

home-assistant bot commented Jan 2, 2025

Hey there @mdegat01, mind taking a look at this pull request as it has been labeled with an integration (influxdb) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of influxdb can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign influxdb Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@RobBie1221 RobBie1221 force-pushed the influxdb_configentry branch from 2c5fd6b to 057e22a Compare January 2, 2025 15:35
Copy link
Member

@joostlek joostlek left a 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 :)

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft January 17, 2025 10:25
@RobBie1221 RobBie1221 force-pushed the influxdb_configentry branch from b9d52c6 to c61519e Compare January 26, 2025 19:23
@RobBie1221 RobBie1221 marked this pull request as ready for review January 26, 2025 19:34
@home-assistant home-assistant bot requested a review from joostlek January 26, 2025 19:34
@RobBie1221
Copy link
Contributor Author

RobBie1221 commented Jan 26, 2025

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).

@RobBie1221 RobBie1221 force-pushed the influxdb_configentry branch from 44e503e to 9584ba0 Compare February 1, 2025 11:10
@RobBie1221 RobBie1221 force-pushed the influxdb_configentry branch from 9584ba0 to d85e04c Compare February 1, 2025 11:21
Comment on lines +409 to 410
if conf.get(CONF_SSL_CA_CERT) is not None and conf[CONF_VERIFY_SSL]:
kwargs[CONF_VERIFY_SSL] = conf[CONF_SSL_CA_CERT]
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

in that case

Suggested change
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):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class InfluxDBConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
class InfluxDBConfigFlow(ConfigFlow, domain=DOMAIN):

Comment on lines +92 to +105
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")
Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Member

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" :)

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Comment on lines +2 to +8
"config": {
"step": {
"import": {
"data": {},
"title": "Import configuration"
}
},
Copy link
Member

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

@home-assistant home-assistant bot marked this pull request as draft February 25, 2025 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants