Support backwards compatibility with Safe.before_deploy attributes

[tim-schilling]

As a follow-up to this conversation, we should support migrations with the old style definitions.

[ryanhiebert]

This is in progress, but it's going to be a big diff with a bunch of semantic and refactoring changes. I had to restructure how the configuration was processed, to make it so that we call the callables once and then determine the resolved status regarding the delays, and that led to rethinking a lot of the structure and semantics.

I'll comment to point out the notable changes, that way you have threads to look at and comment on when your eyes inevitably glaze over. I apologize in advance.

[tim-schilling]

If you're doing a major refactor that may be something we should discuss first.

[ryanhiebert]

Fair enough. It feels close to finished, but that's famous last words. I'll write up a briefing of some notable changes here in a bit.

[ryanhiebert]

I've done some refactoring to abstract some of the implementation details out of the main signal receiver, and to enable callables and delay resolution to only happen one time early in the process. There are a number of other things that came along with that. I may attempt to make separate PRs out of them if it doesn't end up being too much trouble, especially for any part that you feel would be particularly helpful to do so, but I've been just attacking the codebase rather than creating a more reviewable history.

Changes to unreleased or internal functionality

• SafeEnum is renamed to When
  • The choices are When.BEFORE_DEPLOY, When.AFTER_DEPLOY, and When.ALWAYS
  • The safe property of the Safe dataclass is renamed to when to match
• The SafeMigrate enum is renamed to Mode
  • The choices are Mode.STRICT, Mode.NONSTRICT, and Mode.DISABLED
• The SafeMigrationManager is replaced with a SafeMigrationQuerySet and a manager created using models.Manager.from_queryset(SafeMigrationQuerySet)
• the get_detected_map function is replaced by two methods:
  • filter_migration_pairs filters the queryset to the desired migrations
  • get_detected_map takes no arguments and works on the filtering that already happened
• Only write and read SafeMigration records for migrations that are after deploy and have a delay
• Only write SafeMigration records for migrations that are delayed, not those that are blocked
• after_deploy migrations with a delay show the timing of when they can run based on the current time and the database record of when it was seen

And of course

• the safe property can be a callable.
  Should this be just backward compatibility, or a feature? I've written it to be a feature, but I struggle to think of a valid use-case that isn't a foot-gun. Should we special-case it so that the only acceptable callables are the methods on safe?

Changes to released functionality

• Migrations are no longer "protected", only "ready", "delayed", or "blocked". "protected" hasn't really made sense as a category since we added "blocked", but with the addition of the "delay" parameter there's too much semantic overlap to warrant the separate section in the output, and "delayed" is the better category name.
• after_deploy and always migrations can also be blocked if they are after blocked before_deploy migrations.

These two changes both mean that you won't be getting as much information about the safety marking of each migration in the output, but I think that's OK. I can imagine us making a version of showmigrations that could have that information, but that output isn't the most relevant data when you're running the migrations.

Reconsider #38 to change the default

Releasing a major version is probably the right time to reconsider changing the default. With the advent of this new mode of after_deploy migrations that can still be run by safemigrate, it feels like we are recognizing that after_deploy is quite often too strict. Marking an explicit delay and safety marking is ideal, but especially for apps that we don't control like third party and native migrations, that always would be a better default choice.

At this point I'm dubious on the setting to allow it to be configured, mostly because I think that people are unlikely to use that setting. They're probably just going to live with and work around whatever we choose, and follow the instructions we guide them to. This particular choice doesn't feel like a valuable one to expose to them.

Changing the default would increase the priority of having a safemigrate alternative command for makemigrations, that would generate migrations that have a default safety marking, and in this case after_deploy seems quite appropriate as the default, and sets us up for improving that with autodiscovery (#42).

[ryanhiebert]

Because I can't think of a reasonable use-case for dynamic safe attributes, I'm going to restrict that capability to backward compatibility, though I'm not inclined to ever even deprecate the uncalled syntax.

[tim-schilling]

Only write and read SafeMigration records for migrations that are after deploy and have a delay

I disagree with this. I intentionally decided to store that information because it is potentially useful debugging information and it simplifies our implementation.

Only write SafeMigration records for migrations that are delayed, not those that are blocked

A blocked safemigration plan doesn't write anything to the database. This isn't a change from what exists.

after_deploy migrations with a delay show the timing of when they can run based on the current time and the database record of when it was seen

I'm not sure I understand the difference of what already exists.

[tim-schilling]

Regarding SafeMigrationQuerySet I don't see the point from your description here. You're telling me what you did/want to do, but not why. If you're changing it because it feels right, that's something we shouldn't make a habit in terms of maintaining a library. If the new QuerySet methods are being used in other places, then my argument is moot.

[ryanhiebert]

Only write and read SafeMigration records for migrations that are after deploy and have a delay

I disagree with this. I intentionally decided to store that information because it is potentially useful debugging information and it simplifies our implementation.

I think that I may have a different mental model of what this should be doing, based on my intuition captured in #56 to have a version of safemigrate that prefers to start migration delays after deployment is completed. I'll pull this change out of the PR so I can think about more and we can discuss it separately. I'm hoping to avoid things that might make the implementation of #56 have to deal with more edge cases because of this choice.

Only write SafeMigration records for migrations that are delayed, not those that are blocked

A blocked safemigration plan doesn't write anything to the database. This isn't a change from what exists.

This is only true in the default strict mode, where nothing happens if there are blocked migrations. In nonstrict mode, this will stop the delay timer from beginning until the migration is no longer blocked. This may tie into the mental model differences I mentioned above.

after_deploy migrations with a delay show the timing of when they can run based on the current time and the database record of when it was seen

I'm not sure I understand the difference of what already exists.

I think this is just an oversight, and what you intended. But currently it always adds the delay to now, even if the record already exists in the database.

Regarding SafeMigrationQuerySet I don't see the point from your description here. You're telling me what you did/want to do, but not why. If you're changing it because it feels right, that's something we shouldn't make a habit in terms of maintaining a library. If the new QuerySet methods are being used in other places, then my argument is moot.

I'll pull this one out to consider separately. While I agree with your general principle of churn in a library (which is why I feel its important to add the backward compatibility before we release), ISTM that before we release is precisely the right time to evaluate what feels the most correct. If you disagree with the case I will make, then it makes sense to me that implementor's choice wins. But I think that at this stage, even "vibes" should be a good enough reason to change it if there's no other objection (including counter-vibes) than churn.

[ryanhiebert]

I'm just going to drop the SafeMigrationQuerySet changes. I'm sure neither of us care to have that discussion.

[tim-schilling]

@ryanhiebert I don't have a lot of availability for this project right now. Can you reduce your work to what's necessary for backwards compatibility? Otherwise I'm inclined to revert #53 to avoid mismatching docs with what is actually installable.

[ryanhiebert]

I'm working to do that, I think that's wise. It may take me a few days to get it done because I'm at a work offsite atm.

[ryanhiebert]

Tim, I don't know how you've felt by what I've said in this thread, but I feel that I may have caused you some warranted frustration. Thank you for being patient with me.

[tim-schilling]

The frustration was around differing expectations of what it meant to merge #53. I was under the impression that it was reasonable enough to be released. The concerns listed here are valid, but feel like they should have been discussed before 53 was merged.

The other source of frustration isn't something you can control. If the project undergoes a serious re-write, I need to review that for AspirEDU and it's not a priority. So the competing needs of what you want to do with what the company has time for, grinds against each other.

[ryanhiebert]

The concerns listed here are valid, but feel like they should have been discussed before 53 was merged.

It's that challenging balance to me. I trust you as a developer, and recognize that missteps are inevitable and we have the tools to fix the ones prove most troubling, so in my review I didn't do the same depth of review that came up pretty naturally because I was making changes myself. An onerous review that might have caught those earlier might also be a frustrating experience for you as well. But that moment where we're before release, also presents a unique opportunity to avoid releasing something we might wish we had changed, so some things feel more urgent.

Combine that with wanting to get the most bang from cleaning up the little papercuts while I'm at it, and I ended up at a frustrating monster patch.

[tim-schilling]

cleaning up the little papercuts

These are definitely welcome and I appreciated you splitting them off! They are much easier to review and merge piecemeal.

unique opportunity to avoid releasing something we might wish we had changed, so some things feel more urgent.

For what it's worth, I'm fine with making several releases in a row as changes come up. I think I prefer that approach than a release every 6-12 months.

[ryanhiebert]

unique opportunity to avoid releasing something we might wish we had changed, so some things feel more urgent.

For what it's worth, I'm fine with making several releases in a row as changes come up

For this, I was specifically thinking of my reaction to how much you chose to write to the database. I have a pretty strong preference that we'd write and read only what we need, but I've also figured out that by making further schema changes we can enable everything I want to. It just won't be as clean or simple at the end as I would have preferred because once it's released I won't want to break compatibility just to get it a little cleaner or simpler.

But that's not a good enough reason to cause frustration, it just took me a while to figure that out and drop the idea of debating it.

[tim-schilling]

Ahh, I was thinking this was because of my comment about reverting 53 while we sort things out.

[ryanhiebert]

That made sense to me, and why I wanted to let you know that there were likely to be some delays.

[tim-schilling]

I think we're on the same page. I appreciate you letting me know what you're thinking!