django-treebeard
django-treebeard copied to clipboard
django-treebeard
Define policy for backwards incompatible changes
The most recent release, 4.5, has been reported as backwards incompatible by django-cms (#210) and wagtail (#211). I'd like to discuss the problem in this ticket and make decisions on actions and policies to define.
Quick background: treebeard is an OLD and stable project. As such, it's filled with warts and idiosyncrasies for an era that doesn't exist any more. It is also written for a version of Django that nobody uses today, and while for most new releases of Django, treebeard is either compatible or needs only minor tweaks (thanks to the dedication of Django's maintainers for backwards compatibility), the fact remains that Django is an evolving framework, and treebeard hasn't been evolving with it.
Now to have a certain sense of stability, Treebeard has a very comprehensive test suite (#200), and follows Semver for version changes. A backwards incompatible change is, for us, a change in the public API. The PR being discussed in this ticket, #192, didn't expose any breaking changes when I reviewed it, and in fact it added more tests. This is the reason why this release is not tagged as a major one, or it would have been version 5.0 As discussed in #30, stability of this library is paramount, an opinion shared both by @jrief who has been active maintaining this library for years, and me.
Now, despite our commitment to stability, and our caution and respect to Semver, two of treebeard's biggest users report upgrade pains and backwards incompatibilities. I consider this a failure in our process and our testing, and would like to get the involved parties into a discussion on how to prevent this from happening again.
First, about the problem with migrations added (#211), I'd like to add potential migrations as a backwards incompatible change. For this, we need to add a test to make sure that the tables created in our tests don't change from version to version. Is there a library that does this so we can just use it? As a side note, what triggered this problem was adding a default value to MP_Node.depth. I removed it from the model in my local copy of current master, and all the tests pass, so I think this change can be reverted, unless somebody has a good reason not to? (pinging @johanneswilm and @obayemi for their input).
Second, about the deepcopy error (#210), I'm going to have to dig a bit more on this, but what occurs to me as a preventive measure is adding a quick sanity test run for projects of certain size that depend on treebeard's stability. Looking at pypi I see that we have maintainers from wagtail (@gasman), django-cms (@yakky) and django-oscar (@mvantellingen and @solarissmoke). If there is guidance on how to have a quick sanity test of developer branches of treebeard against these projects, it would help us fulfill our stability expectations.
Note that for both cases, I'm proposing setting policies and verifying them with running tests. The only way to know for sure the policies are not just words in a readme :)
For this, we need to add a test to make sure that the tables created in our tests don't change from version to version.
Both Wagtail and Oscar use this approach, of having a test that checks for missing migrations in a test project, which fails if there are model changes for which new migrations have not been committed. Treebeard could do something similar. I'm happy to make a PR to add this if there's agreement that it would be useful.
If there is guidance on how to have a quick sanity test of developer branches of Treebeard against these projects, it would help us fulfill our stability expectations.
I'm not sure how easy this would be to do, given that we'd have to execute functional code in each project before we can determine that we've not broken anything, and it would require more effort that I think would be justified to keep that up to date, given that projects like Wagtail are very active.
I think the migration issue can be addressed quite easily with the approach described above - and combined with a more conservative approach to applying semver rules (which does come down to policy/judgement at the time of cutting a release), would have avoided this particular issue.
The other thing that I think would have helped in this case is to have cut an unstable (beta) release of 4.5 a few weeks before releasing the stable version - this would have increased the chances of the issue being picked up before it hit production builds. Maybe there's another policy element here to ensure this is always done going forward.
As a side note, what triggered this problem was adding a default value to MP_Node.depth. I removed it from the model in my local copy of current master, and all the tests pass, so I think this change can be reverted, unless somebody has a good reason not to? (pinging @johanneswilm and @obayemi for their input).
I would have to look into that. But now that 4.5 has been released already, wouldn't reverting this mean we have to publish another major version? Is this what you are planning on doing in version 5.0?
I think the plan of making a beta version first and then manually or automatically testing that with the three major projects mentioned above before making a final release sounds like a good solution. And if it is important for wagtail not to have extra migrations due to minor version upgrades, then I think it would be reasonable to make a policy to only have those in major version upgrades.
~~Also, whenever something like this happens, tests should be added for those things that are failing (in this case the deepcopy issue).~~
Edit: It actually looks to me like the bug is in Django documentation/Django-cms, as @tabo pointed out in #210 .
@tabo Is this the commit you want to revert? https://github.com/django-treebeard/django-treebeard/commit/b131b85ed4493ed855b8b4e35ae5379e661f15ac If so, maybe the author should be contacted as well?
@johanneswilm Nice catch! So it seems that even when @JamesMaroney implemented this initially, he wasn't sure it was needed at all:
Least sure about this change - not sure why this became needed, which worries me. Without it, depth was None causing a failed DB NOT NULL constraint failure
As I mentioned, I confirmed that in my local copy of current master, all tests pass after reverting this commit. The constraint failure was probably unrelated.
About cutting a new release, I would be willing to make it on the 4.x branch and establish the migrations policy after making this release.
Nice catch! So it seems that even when @JamesMaroney implemented this initially, he wasn't sure it was needed at all:
As far as I understand he was sure he DID need it, but he was not sure WHY. I am guessing that this could have to do with a specific database backend in a specific Django version that would fail if you saved a PositiveIntegerField with no modifications to it. Are we sure that is no longer the case on any of the Django versions we support?
Could we maybe have a 4.6-beta release first with this reverted. Then I can run that against our code and see if it works.
Looking around the net, I see that there are several instances of PositiveIntegerField(null=True). I am guessing that was another way of solving that issue.
I agree that you appear to be following your own processes but the tests have potentially missed the projects use cases. The change in question brought in two tests and for a change of this size of would have expected boundary tests for the changes to the Node creation logic as this is fundamental logic to this package. A copy unit test should also be present: https://github.com/django-treebeard/django-treebeard/pull/192/files#diff-0fd89484e43d8d6ded307187a21557b6cb6cd9e3cf89147cdb08d751bb918671R731
We can potentially role a generic test for the django-cms uses cases that is not django-cms specific and generalises a common use case that could be used by any other projects.
The thing is that this part did not introduce a fundamentally new logic and is just following general django rules. The bug looks like an oversight in the django documentation and in django-cms. The PRs for adding the functionality have been waiting for many years and there have been almost no releases of django-treebeard in years. So I think it's about as conservative as it's possible to get.
I think the solution you propose, @Aiky30 , of providing tests for all the parts you use makes a lot of sense.
In hindsight it might have been better to just name this release 5.0.0 even though we were not aware that it broke anything, just because the amount of changes was large enough that it we should have it could introduce breaking changes that we would not find.
Alternative, it would have been a good idea to do a beta release with manual testing against all major projects using this.
@johanneswilm The logic fundamentally changes when a node is created, that's a core function of django-treebeard. Any changes to core functions should contain strict tests and in theory is a major change, the issues that both big projects consuming this package have proved this. Luckily django-cms 4.0.x is less dependant on django-treebeard as only pages use it vs django-cms 3.x where it is used for plugin and page trees, a custom implementation was added for plugins. If this kind of issue was to occur again we would look to remove any dependancy in future django-cms versions.
It's changing yes, but it's still using documented django functionality. So what test would have let it fail? As far as I can tell, the test would itself have to have a bug in it in order to fail. So I'm not convinced that more tests would have helped.
However, testing against software packages is something else. That would indeed have shown the issue.
@Aiky30 @tabo Can we contact those behind the django documentation to figure out if it is indeed an issue with the django documentation or whether there is something we have misunderstood?
If there is guidance on how to have a quick sanity test of developer branches of treebeard against these projects, it would help us fulfill our stability expectations.
Wagtail already tests against Django master as part of its Github Actions matrix, marked as continue-on-error so that if Django does make a breaking change it doesn't block Wagtail development in the meantime. I'd be happy to add a similar test run against treebeard's git master as well if that helps, although having that running inside wagtail's test suite obviously isn't the most visible place to check when developing treebeard. I guess there's nothing stopping us from basically cloning wagtail's Github Actions script inside the treebeard repo, so that we're running wagtail's test suite against current checkouts of wagtail and treebeard.
The other thing that I think would have helped in this case is to have cut an unstable (beta) release of 4.5 a few weeks before releasing the stable version - this would have increased the chances of the issue being picked up before it hit production builds.
I think this is worth doing too, although my advice would be not to set your expections too high that people will actually try it out :-) For Wagtail, we make a release candidate around 2 weeks before the final release, and while we sometimes catch bugs in that period, inevitably the majority of bug reports still come in as a flurry in the days after the release, making us scramble to get a .1 release out... And that's on a project where we have a decent ability to generate marketing hype about upcoming features - I suspect that's going to be a harder sell for treebeard.
Is it at all feasible to create a Github Actions test for django-treebeard that pull a stable release of django-oscar, wagtail, or django-cms and tries to run its tests using the checkout version of django-treebeard? Or would that require too much setup that cannot be done within the scope of Github Actions?
I'm in general against making beta/testing versions and hoping for them to be tested. That's not automation. That's not an assurance. I'd rather have a repeatable process that tells us that we're not breaking dependent projects even when all our tests are passing. I'd rather have a PR from a contributor to be checked that it's not breaking projects before it's merged into master. That's automated and visible, and needs no human effort.
If we decide that the dependent projects will test against treebeard master instead, like @gasman suggests, that's also an option. It wouldn't catch problems before merging a PR into master, but it would help us prevent a bad release.
The purpose of this ticket is to have this discussion. Personally I would prefer to have this testing in treebeard itself, for the validating-a-PR-before-merging I was mentioning. We wouldn't need to run the whole test suites of the dependent projects, just a subset of the best tests that are excercising treebeard calls.
About the tests that validate that we're not adding migrations (#211), question for @solarissmoke: could we use the models in tests.models for this? They're already there, and they use a big variety of features. It would save us the work of creating a separate app with models just for this.
Could we use the models in tests.models for this?
@tabo yes: https://github.com/django-treebeard/django-treebeard/pull/213 .