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

Config flow for the Flux integration #94394

Closed
wants to merge 75 commits into from

Conversation

schelv
Copy link
Contributor

@schelv schelv commented Jun 10, 2023

Proposed change

I've made some key improvements to the Flux integration:

Improvements:

  1. Simplified Installation: Flux can now be installed and set up directly from the UI, removing the need for yaml configuration.
  2. UI Parameter Consolidation: I've consolidated brightness and disable_brightness_adjust into one UI option, "Adjust Brightness".

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

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

To help with the load of incoming pull requests:

@home-assistant home-assistant bot added cla-signed config-flow This integration migrates to the UI by adding a config flow has-tests integration: flux Quality Scale: internal labels Jun 10, 2023
@schelv schelv marked this pull request as ready for review June 11, 2023 16:13
homeassistant/components/flux/config_flow.py Show resolved Hide resolved
homeassistant/components/flux/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/flux/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/flux/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/flux/strings.json Outdated Show resolved Hide resolved
homeassistant/components/flux/switch.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft November 27, 2023 13:42
@schelv schelv marked this pull request as ready for review November 28, 2023 22:52
@home-assistant home-assistant bot requested a review from emontnemery November 28, 2023 22:52
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Jan 31, 2024
@joostlek
Copy link
Member

Bumping this one to be on my list again

@github-actions github-actions bot removed the stale label Jan 31, 2024
@emontnemery
Copy link
Contributor

emontnemery commented Feb 14, 2024

I discussed this PR with @balloob , @frenck and some other core developers. The concern is that the flux integration has known issues and limitations and we don't want to promote it by adding a config flow to it at this point.

There are also a couple of well maintained and popular custom integration with more options:

@balloob thinks the functionality implemented by flux is very useful, which lines up with the popularity of the custom integrations, but he suggested a deeper integration with Home Assistant than an integration. Maybe a built-in option for lights with adjustable color-temperature to make them automatically adjust the color temperature during the day? Going in this direction would require an architecture discussion.

Another view (which I personally do not share) is that the functionality provided by the flux integration is too simple and could just be replaced with a blueprint, such as this one https://community.home-assistant.io/t/advanced-circadian-lighting/383543

Until the way forward is clear, we do not want to merge this PR.

@schelv
Copy link
Contributor Author

schelv commented Feb 25, 2024

@emontnemery Thank you for the comment.

In an attempt to turn this reply into a concise and coherent story, I've chosen to structure it as follows:
I will start by providing some context; Explain what in my view the goal should be, and what the most feasible approach is to reach it.
Then I will reply/address/discuss the concern and alternatives that were mentioned.

Some context + chosen approach
My opinion is that light control (including the functionality provided by flux) should be easy to use by users of all knowledge levels. (If installation is required, it should also be easy)
It currently is not easy to configure/use this in home assistant.

I know that it can be very difficult to make something "easy to use".
I do have (big) ideas on how things can be improved, but do not have the knowledge and experience required to effectively implement them. (and getting it merged might also be a challenge 😉).
Contributing to this project becomes more realistic if I make an existing thing "easier to use" with multiple smaller changes.

That is why I decided to improve this integration.
In its current state it probably isn't good, but:

  • it has potential to be easily installable (because it is in the core),
  • by iteratively adding multiple small improvements, it might even become decent.

The first small improvement I had in mind was this one: #93181
But I found out I was the third person to propose exactly this change.

Apparently making yaml changes is not allowed, and a config flow is needed. (See #86201 (comment))
That is the reason why this merge request exists.

Suggested alternatives.

I will address/discuss alternatives that were mentioned, and relate them to the context provided above.

There are also a couple of well maintained and popular custom integration with more options:

The integrations seem really good, but sadly they need to be installed using HACS.
HACS itself is not easy to install by users of all knowledge levels. ("clear your browser caches", having/creating a Github account)

@balloob ... suggested a deeper integration with Home Assistant than an integration. ... Going in this direction would require an architecture discussion.

As stated earlier I don't feel I have the knowledge to implement architecture changes.
But I do like the idea of deeper integration. Not having to install/add an integration is definitely very easy.
Maybe the deeper integration could be a (very future) continuation of flux (without the issues and limitations).

The Concern

I discussed this PR with @balloob , @frenck and some other core developers. The concern is that the flux integration has known issues and limitations and we don't want to promote it by adding a config flow to it at this point.

I don't know what the "known issues and limitations" of flux are.
But I can understand that you don't want to promote an integration if it has issues.

Proposed way forward:

  • Someone makes a list of the "known issues and limitations" (knowing them is a prerequisite).
  • Identify which issues and limitations need to be addressed before flux becomes config-flow-worthy.
  • Someone addresses the issues and limitations that need to be addressed before flux becomes config-flow-worthy.
  • flux becomes config-flow-worthy

Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Apr 25, 2024
@github-actions github-actions bot closed this May 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2024
@emontnemery emontnemery reopened this May 13, 2024
@home-assistant home-assistant unlocked this conversation May 14, 2024
@github-actions github-actions bot removed the stale label May 14, 2024
@frenck
Copy link
Member

frenck commented Jul 6, 2024

Hi there @schelv

As pointed out correctly above, we indeed don't want to move forward with this at this point.

You ask about the issues with this integration; there are quite a few (especially when working with multiple light sources), but also feature completeness.

From as user perspective point of view, eventually, we don't want this to be an integration that provides a "device automation"-as-integration, but a native, built-in part of just lights in Home Assistant.

In the end, wrapping a light in an integration to automate stuff like this is odd. For that same reason, we don't want to level up a few more of these integrations (like alert and device_sun_light_trigger).

Nevertheless, thanks for being willing to contribute ❤️

../Frenck

@frenck frenck closed this Jul 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed config-flow This integration migrates to the UI by adding a config flow has-tests integration: flux Quality Scale: internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants