Make app config json migration not be skipped by patch release

[chiiph]

We released 4.2.3 by cherrypicking commits. Some of them brought a migration with number X. Out of chance we realized that we have a migration with number X-1 that was added but not released in 4.2.3. This migration won't be added afterwards if

Checklist for submitter

If some of the following don't apply, please write a short explanation why.

• Changes file added (if needed)
• Documented any API changes
• Added tests for all functionality
• Manual QA for all functionality

Tested starting at 4.2.3, then going to 4.2.4 and then to main <-- app_config_json is not there.
Then did the same but instead of main used this branch. All looks good.

[chiiph]

This is a patch, we need a better system. We will run into this again.

[mna]

Agreed, this is fragile.

[codecov-commenter]

Codecov Report

Merging #2034 (7dd421f) into main (6a3458f) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2034      +/-   ##
==========================================
- Coverage   47.69%   47.67%   -0.03%     
==========================================
  Files         220      220              
  Lines       20988    20988              
==========================================
- Hits        10011    10005       -6     
- Misses       9775     9779       +4     
- Partials     1202     1204       +2     

Impacted Files	Coverage Δ
server/datastore/mysql/hosts.go	80.42% <0.00%> (-0.80%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a3458f...7dd421f. Read the comment docs.

[zwass]

I'm not sure this is sufficient -- If someone migrated to 4.2.4 and then made some changes to the app config, I think this migration will cause the old app config to overwrite those changes. Does that seem like the case?

[chiiph]

I'm not sure this is sufficient -- If someone migrated to 4.2.4 and then made some changes to the app config, I think this migration will cause the old app config to overwrite those changes. Does that seem like the case?

4.2.4 doesn't have app_config_json, it's still in the old one.

[zwass]

Sounds like I misunderstood. Thank you.

[chiiph]

Sounds like I misunderstood. Thank you.

It's definitely confusing when we add patch releases to the mix. Not sure if it's better to iterate on goose to address these issues, or see what other frameworks are out there and migrate to that. Any thoughts there?

[zwass]

I am open to both possibilities. By the nature of migrations though, I'm not sure we can address this via the framework? Definitely curious if anyone has come up with good options.

FYI we are on a fork of Goose from long ago -- the current Goose may be better.

[mna]

IIRC the original Goose (the one that was on bitbucket I think) saw very little activity at some point, and a number of forks took off. I think there's one that allows converting the timestamps in the filenames to incrementing numbers, which is only marginally better. I personally haven't seen an approach that really addressed the kind of issue discussed here, though.

[chiiph]

https://github.com/pressly/goose#hybrid-versioning attempts to addresses our issue.

[mna]

Yeah, that's the thing I was referring to. Not sure it really addresses this particular case though, but that's only from a cursory glance at what they do, I never used that approach.

[chiiph]

pressly/goose#63 (comment)

^ having a tree of migrations sounds reasonable. As long as we define an order, we can traverse that tree and check that we have all the nodes in the order we defined.

[zwass]

Does that mean that any PR with a migration will need to be manually edited to have a different dependency each time a migration is merged in a PR?

[chiiph]

Does that mean that any PR with a migration will need to be manually edited to have a different dependency each time a migration is merged in a PR?

No, it would be like git. The migration would have a parent migration. Applying a migration would be:

1. Apply root migration
2. Apply all the migrations that have the root as parent
3. For each of the migrations in the step ran before, apply the migrations that have that as parent.
4. If no more migrations, done. Otherwise, go to three.

So it would be a breath first traversal.

[chiiph]

So a migration would have to have the oldest possible parent... which could be annoying to find by hand. You would use as a parent the last released migration, or one that is newer, if the migration depends on that newer one.

That way, at any point, we can release a patch with whatever we cherry pick, we just need to make sure we keep the tree chain complete, which we can probably script out as a CI check.

[mna]

You could still have conflicts in the migrations at the same tree depth (e.g. 2 PRs start from the same parent migration, so this is their "dependent"/parent migration, one PR adds a migration that alters an existing column, the other drops it or something else that conflicts with the other). In my experience (and the scenario in pressly/goose#63 (comment)), conflicts often arise from two PRs starting from the same migration, and one gets merged before, invalidating the other (or creating conflicts). Also, how would you keep track of applied migrations in such a tree (e.g. vs a sequential number)? I guess one way would be to store the hash of the migration, or the name of the migration file (i.e. multiple rows, check for each file if it was applied, versus just storing the last applied version?).

In any case, not totally clear to me how treating migrations with dependencies/as a tree fixes those kinds of conflicts.

[zwass]

Is this possibly a problem that would be better solved by making some tooling that ensures we cannot release a branch that includes only some (not all) of the migrations up to a certain point on the main branch?

[mna]

I think a CI job could definitely detect such issues and prevent merging in those cases (not sure if that's exactly the same thing you're talking about as I'm not familiar with the release process, but I really like the idea of a tooling-based/"static analysis" solution).

[chiiph]

In any case, not totally clear to me how treating migrations with dependencies/as a tree fixes those kinds of conflicts.

Yeah, it wouldn't solve all the problems.

Is this possibly a problem that would be better solved by making some tooling that ensures we cannot release a branch that includes only some (not all) of the migrations up to a certain point on the main branch?

Sometimes we need to release only some of the migrations. Eg:

• implement new feature A, this has migrations
• find out of a bug in the previous release, add a migration to fix the issue

You would want to still "hide" new feature A until the other release and not release it in a patch release.

Maybe if we implemented something that checked out each tag one by one, ran prepare db, and checked that the resulting schema is equal to the test schema at that point, it would be enough.

[chiiph]

Note that this cannot be done at merge time, because the patch releases are cut sometimes in new branches with cherry-picked commits. But we can run this when we push a tag, or by hand as part of the release process.