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 feature flag concept and screen #4760

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

TimoPtr
Copy link

@TimoPtr TimoPtr commented Oct 27, 2024

Summary

This PR introduces a feature management system to streamline future development, laying a flexible foundation designed to adapt as needs evolve. A new UI for managing feature toggles is available in the troubleshooting menu, allowing features to be controlled directly from the app.

Additionally, this update includes unit tests for feature-related functionality and a CI job to execute them automatically on each PR.

This design takes inspiration from Baracoda’s blog on trunk-based development.

Screenshots

dark.mp4

Any other notes

This is my first contribution to Home Assistant, and I tried to keep the existing structure and conventions.

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @TimoPtr

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@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 October 27, 2024 08:46
Comment on lines +1302 to +1305
<string name="feature_flag">Feature flag</string>
<string name="feature_flag_summary">Manage feature flags for the app (advanced users only). Proceed with caution, as this area contains experimental features that may affect stability.</string>
<string name="string_feature_flag_update">Update</string>
<string name="string_feature_flag_cancel">Cancel</string>
Copy link
Author

Choose a reason for hiding this comment

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

I don't think I can add this new keys myself in Lokalise.

Copy link
Member

Choose a reason for hiding this comment

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

If your PR is accepted and merged, Lokalise will pick up the new strings automatically

@@ -67,6 +67,28 @@ jobs:
- name: Validate Lint
run: ./gradlew lint

unit_tests:
Copy link
Author

Choose a reason for hiding this comment

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

This is a very basic job, that most probably should evolve over time.

@TimoPtr TimoPtr marked this pull request as ready for review October 27, 2024 09:07
@bgoncal
Copy link
Member

bgoncal commented Oct 29, 2024

Could you bring some examples where you see this feature flag engine relevant for future HA features? Also have in mind the open source contributors and their interact with it.

On the iOS side I haven't felt the need for feature flags yet (even though I worked with a similar approach in past companies I worked on), so I am curious about your perspective.

@TimoPtr
Copy link
Author

TimoPtr commented Oct 29, 2024

A bit of background about why I brought this to the project: initially, I was annoyed with the dialog in the Launcher when my connection was off, and I thought I could make it nicer with a proper screen and a nice icon. But while looking at the code, I found some complex code that could be simplified. I then told myself that I could bring this feature toggle capacity to the project without breaking everything so that later, in a second PR, I can use it to bring this new screen while keeping the other one until we agree if it is the way to go.

image

Generally speaking, I think that large-scale refactors can sometimes be challenging for reviewers/writers, and breaking them down into smaller, manageable PRs that are feature-flagged would allow changes to go through CI and get faster feadback on the design without impacting the main application. This approach keeps PRs digestible while making continuous improvements possible and avoids long-lived branches.

Another potential use is A/B testing. While this PR doesn’t enable A/B testing directly, the design is extensible for that purpose. Testing features selectively before a full release would allow us to gather valuable feedback from the community and validate changes.

Considering I’m still getting familiar with the codebase, these examples may not be the most precise, but here are some potential areas that could benefit from feature flags:

  • Refactoring the dialog in Launcher Activity: A feature flag would allow us to gradually improve error handling in the launcher without impacting the whole app. For example, we could move away from displaying dialogs with an Android WebView error page in the background and instead handle dialogs in a more user-friendly way, step by step, without breaking existing behavior. Also, migrating to compose. Reference

  • Migrating from Fragment to Compose Navigation: This transition could be handled gradually, allowing contributors to enable Compose-based navigation while retaining the legacy fragment navigation, making incremental migration smoother. Reference

  • Experimental Feature Testing: For instance, a feature toggle could control experimental Gemini interactions, allowing contributors to incrementally build and test functionality before a full rollout.

  • UI Validation through Mock Devices: Flags could enable UI changes to be tested against mocked devices, allowing a first level of validation when hardware is unavailable.

  • A/B Testing for UI Redesigns: If the app's design changes in the future, feature flags at the theme level could enable selective rollout and user feedback collection (A/B testing), ensuring design changes are well-received.

  • Contributor Collaboration and Testing: For open-source contributors, feature toggles can facilitate discussions where opinions differ. Testing new ideas or designs in parallel can be a practical way to make data-driven decisions on debated features.

I might be biased by my past experiences, and I would happily discuss it if you think it won't bring value to HA.

@bgoncal
Copy link
Member

bgoncal commented Oct 30, 2024

I see the value on it and I apretiate you sharing your past experience, I'd say this becomes interesting as soon as we have a real need for it, for now we focus on small PRs, with single purpose, that can be merged without breaking the rest of the code, for a big refacture, perhaps a feature flag engine could be helpful, on the other hand it's usually possible to breakdown refactors as well.

About A/B testing, we don't collect user data, as one of our privacy principles from the Open Home Foundation/Home Assistant.

We have a call today and we can discuss this more in depth, meanwhile @jpelgrom @dshokouhi are the right people to decide if this can be helpful for android right now or perhaps hold for future use cases.

@jpelgrom
Copy link
Member

While I do see the value of such a feature in general and for the developer while working on PR, I still don't fully see how it would benefit Home Assistant.

  • The examples given for (partial) refactoring suggest to me you want to merge code that would otherwise break the app in some way and hide it behind a flag, instead of splitting it up into smaller chunks that don't break?
  • As Bruno mentioned, Home Assistant has a strong stance on privacy. Using it for A/B testing as you suggested or random bits of AI are not really compatible as I see it.
  • We use pull requests for discussions where opinions differ, not merging changes immediately and discussing later.
  • There already are not a lot of contributors to the companion app. I feel like introducing a framework like this and mandating it would increase the barrier to entry, with very little gain (the app is practically already able to release from the main branch at any time, and there are limited conflicts.)

At a quick glance, the code looks fine. Did you also write/contribute to the blog post you've linked as it is from your current company?

@TimoPtr
Copy link
Author

TimoPtr commented Oct 31, 2024

I was more thinking about the case where the refactoring is done partially. Imagine that we redo a big Activity with multiple fragments to Compose with the navigation library. In that case, I would expect to have a first PR with a feature flag defined, and then subsequent PRs for each fragment transition to the new system. Finally, one last PR would remove the flag and the old code.

With this approach, everything can be merged quickly to main without losing any features. I don't know the code base well enough to say that this case will happen.

When it comes to the A/B testing, I'm fully aligned with the privacy concern, and I wouldn't want my data collected. I was more thinking about getting feedback on GitHub or Discord from the users about, let's say, a new UI that we want to test. I guess that there is inside the community users that would be ok testing new things and give us feedbacks.

random bits of AI are not really compatible as I see it

I'm not sure I follow. Are you referring to my point on Gemini? If so, I was thinking about the local usage of LLM models. Today, I'm experimenting with running "small" LLMs locally, as the latest flagship devices come with significant power and dedicated hardware. It could be nice to leverage that. Again, I'm aligned with the privacy concerns, which is why I would prefer to keep the LLM running locally on open source libraries that is not collecting data.

I fully agree that forcing people to use this framework can be too complex. It probably shouldn't be forced. Maybe sometimes the reviewers could suggest splitting the PR if it's too big using feature flags or if the feature needs testing before broader deployment.

When it comes to the blog post, in my current position, I'm supervising the writing in the blog, contribute through reviews and approve the publications.

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