-
Notifications
You must be signed in to change notification settings - Fork 4
Description
The current CI tests against the v3 branch of the OpenFlexure Microscope, which is intended to make sure changes in LabThings don't break the microscope.
We've previously considered keeping a running branch of the OFM codebase, so we could adapt it to new non-backwards-compatible features and keep the test passing. However, I'm not sure that's the best strategy. Testing against a modified branch means labthings-fastapi/main will frequently not be compatible with openflexure-microscope-server/v3 which is unhelpful, and leads to us having to develop in lockstep. It also means that we'd be attempting to activate all the new features at once, which feels like it makes life unnecessarily difficult.
I'd like to propose a different strategy, which is to use feature flags and DeprecationWarning consistently: when a new feature is added, it will either be backwards-compatible, or it will be disabled by default. The test-against-ofm-v3 job will use the default settings, so it should pass against the current OpenFlexure codebase in v3 without changes.
If enabling a feature would require changes to the OFM codebase, we'll open a MR there. That MR will enable the feature and implement any required changes. I don't think we should make this a hard requirement for merging PRs here so long as nothing breaks with the feature disabled, but as most of the features we add to LabThings are driven by OFM development anyway, it's something we will want to do most of the time either before or after it's merged. #293 allows pull requests to test against both v3 (i.e. the unmodified microscope, with new features turned off) and against a new feature branch (i.e. a modified OFM branch with the feature enabled).
Once there's been a release of LabThings that's been adopted by the OFM codebase (which will go in with new features disabled), we can:
- rebase any merge requests to OFM that enable feature flags (this will ensure that
labthings-fastapiis being installed from a release rather than a branch) - merge them one by one
I think this has some significant advantages over the approach proposed in this PR. Specifically:
- It's very clear when breaking changes are introduced, because we now expect
test-against-ofmto pass most of the time. - We get into the habit of explicitly linking to a feature branch when LabThings features would need changes in the OFM in order to be enabled.
- New features can be adopted and tested one-by-one.
- It minimises the number of open MRs and extra branches we need on the OFM repo.
- New features are small, focused MRs that target
openflexure-microscope-server/v3with no need for intermediate LabThings-related branches. - We don't get accustomed to merging PRs where the
test-against-ofmjob(s) fail.
I think this strategy minimises the risk of upstream changes causing problems for the OFM, while also keeping the ongoing development overhead to a mangeable level.
Implications for versioning/milestones/feature schedule
I think, while we still have a major version of zero, it would be reasonable to add backwards-compatible features (including changes that are behind a feature flag) with patch releases. I think it's also reasonable for patch releases to add DeprecationWarnings to things that will be enabled by default in the next minor version.
Breaking changes should bump the minor version number. The principle is that the OFM should be able to update patch releases without worries that LabThings will be broken.
I'd propose that any feature that's introduced behind a feature flag may be enabled by default in the next minor release. Of course, if downstream code isn't ready, it may be disabled explicitly. If we plan for an optional feature to become the only option in the future, we can raise a DeprecationWarning if it's disabled by downstream code.
I think it would be good to aim for flagged features to be enabled by default but remain optional (with a warning) for at least one minor release cycle: this provides a useful measure of stability for downstream code and stops updates being too rushed, without tying our hands too much.