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

Add Aladdin connect config flow #68304

Merged
merged 93 commits into from May 13, 2022
Merged

Conversation

mkmer
Copy link
Contributor

@mkmer mkmer commented Mar 17, 2022

Breaking change

The aladdin_connect integration migrated to configuration via the UI. Configuring aladdin_conenct via YAML configuration has been deprecated and will be removed in a future Home Assistant release.

Your existing YAML configuration is automatically imported on upgrade to this release; and thus can be safely removed from your YAML configuration after upgrading.

single step config flow:
image

Proposed change

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

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
  • The code has been formatted using Black (black --fast 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@homeassistant

This comment was marked as outdated.

@epenet
Copy link
Contributor

epenet commented Mar 17, 2022

You should an import flow, so that existing YAML configurations are automatically imported into a new config entry.

Copy link
Contributor

@tkdrob tkdrob left a comment

Choose a reason for hiding this comment

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

Yaml config import is required when moving to a Config Flow. Please look at other integrations that have an import step to see how it is done.

@homeassistant

This comment was marked as outdated.

@homeassistant

This comment was marked as outdated.

@mkmer
Copy link
Contributor Author

mkmer commented Mar 17, 2022

You should an import flow, so that existing YAML configurations are automatically imported into a new config entry.

Any pointers to an example? I can't seem to find one :(

@gjohansson-ST
Copy link
Member

Any pointers to an example? I can't seem to find one :(

https://github.com/home-assistant/core/blob/dev/homeassistant/components/sensibo/climate.py#L84-L93
https://github.com/home-assistant/core/blob/dev/homeassistant/components/sensibo/config_flow.py#L78-L81
For a very simple one

@mkmer
Copy link
Contributor Author

mkmer commented Mar 17, 2022

@gjohansson-ST Thanks for the references, works like a charm!

Copy link
Member

@gjohansson-ST gjohansson-ST left a comment

Choose a reason for hiding this comment

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

Looks like it's coming together nicely.

@mkmer
Copy link
Contributor Author

mkmer commented May 1, 2022

There's a LOT to learn... actual running code appears to only be 30% of the job :)
I have had it running in my "production" instance for quite a while now... but most of the edge conditions never actually happen.

@mkmer
Copy link
Contributor Author

mkmer commented May 2, 2022

One more question related to the reauth flow. In working out the lint error, I removed this from my version. I think that's a pointer to the entry.data (or it's the values already in the data?), but I don't believe my integration uses it if it's a pointer... I don't see anything similar in the example code.

I guess the question is: did I do that right? :)

@gjohansson-ST
Copy link
Member

One more question related to the reauth flow. In working out the lint error, I removed this from my version. I think that's a pointer to the entry.data (or it's the values already in the data?), but I don't believe my integration uses it if it's a pointer... I don't see anything similar in the example code.

You changed it to this instead which results in the same thing here now when you specify both username and password. **self.entry.data refer to entire dict so you only need to specify the changes but for only two key's it should be fine to specify them as you did now.

Copy link
Member

@gjohansson-ST gjohansson-ST left a comment

Choose a reason for hiding this comment

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

Some bits and pieces.
Seems close to done

homeassistant/components/aladdin_connect/cover.py Outdated Show resolved Hide resolved
homeassistant/components/aladdin_connect/strings.json Outdated Show resolved Hide resolved
tests/components/aladdin_connect/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/aladdin_connect/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/aladdin_connect/test_cover.py Outdated Show resolved Hide resolved
tests/components/aladdin_connect/test_init.py Show resolved Hide resolved
@mkmer
Copy link
Contributor Author

mkmer commented May 12, 2022

I believe CI errors are because of a needed rebase? Looks like stuff that failed is not part of this change? Try again?

@Kane610 Kane610 merged commit f6600bb into home-assistant:dev May 13, 2022
@Kane610
Copy link
Member

Kane610 commented May 13, 2022

Congratulations @mkmer 🎉

@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2022
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Please address the comments in a new PR. Thanks!

@mkmer mkmer deleted the aladdin_connect branch May 16, 2022 12:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aladdin Connect fails to setup after upgrade to 2022.2.0
8 participants