DEV: feature-flag implementation at app-level

[kecnry]

NOT (YET) INTENDED FOR REVIEW/MERGE

UP FOR DISCUSSION

This pull request implements one possible re-usable approach to feature-flags. The idea here is that we can merge incremental PRs directly into main without making an incomplete feature user-facing, and then switch the feature to be enabled (or delete the applicable code blocks) down the road. This is an alternate to the long-lived feature branches as we've seen with image rotation that can become difficult to manage.

#2664 had implemented a single-use feature-flag, which inspired this more general approach. kecnry#7 then shows how that would be modified to use the implementation proposed here.

The general idea here is that anything that is not ready for use yet, can be disabled by default, but still exposed for developer testing with something like:

viz.app._ff_enable('my-feature')

or within test blocks temporarily with:

with viz.app._ff_temporarily_enabled('my-feature'):
    # in here the feature is enabled, outside it is not.

We then can also put v-if statements anywhere in vue to hide/alter the applicable sections of the UI, or if app._ff_is_enabled('my-feature') in python to raise NotImplementedErrors, etc.

Milestones and changelogs: if we go in this direction, how should the changelog and milestones be handled? Can we milestone these into a separate section of the changelog entirely and then somehow just go move the necessary entries? Should we have a milestone for each active feature we're working on so that a specific release version doesn't have to be targeted yet?

[pllim]

A test failed: TypeError: Application._ff_is_enabled() missing 1 required positional argument: 'feature'

My concern is that this would be one more thing that we have to keep track of, and maybe sometimes we would forget to use it. I cannot tell just how useful this would be going forward. If we ever use this on one or two things, is it worth it? When/if Jdaviz becomes feature complete, do we then delete this?

Also, I don't think the rotation effort would have benefited from this. It involved changing Imviz reference data handling across the whole app.

[kecnry]

I cannot tell just how useful this would be going forward. If we ever use this on one or two things, is it worth it?

That's a fair question we need to consider. But we do have a few efforts starting now that I think would benefit from this 🤷‍♂️

When/if Jdaviz becomes feature complete, do we then delete this?

We could, it doesn't really carry too much overhead though, I don't think. My suggestion would be to permanently enable features when they're complete rather than just flipping them to enabled by default, so then we could just basically revert this PR to remove it entirely if we want.

Also, I don't think the rotation effort would have benefited from this. It involved changing Imviz reference data handling across the whole app.

I agree - and so alternatively we could say that the things we have coming up could still use feature branches and hopefully just not balloon to be quite so messy?

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.53%. Comparing base (9140858) to head (1cecd52).
Report is 699 commits behind head on main.

Files with missing lines	Patch %	Lines
jdaviz/app.py	88.23%	2 Missing ⚠️
jdaviz/core/template_mixin.py	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2667      +/-   ##
==========================================
- Coverage   91.53%   91.53%   -0.01%     
==========================================
  Files         161      161              
  Lines       20106    20130      +24     
==========================================
+ Hits        18404    18425      +21     
- Misses       1702     1705       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[pllim]

then we could just basically revert this PR to remove it entirely if we want.

Theoretically yes, but I really don't think anyone but you would be able to do it cleanly. What if you are reassigned to something else? Then we have this machinery that no one understands completely.