Sync changesets in the background instead of in ApplyCampaign
Created by: mrnugget
This changes the ApplyCampaign
mutation so that it doesn't sync imported changesets in the request path. Instead it flags the changesets as unsynced and enqueues them to be picked up the reconciler. The reconciler then syncs them.
Instead of a enum I simply used a boolean flag, which gives us a handy default value and makes the migration easy.
Since changesets can now exist in the DB without having any external data, I had to loosen the constraints in the API a bit: Title
and Body
are now nullable (because we don't know what the title/body are until we've synced the changeset, of course).
The downside is that a freshly added but unsynced changeset looks like this in the UI:

It's cool that the processing works already, but the parentheses around the external ID look a bit weird...
@eseliger should we expose "Unsynced" in the GraphQL API so we can check for that? Or something like "SyncStatus"? Although, again, an enum might be overblown.
Merge request reports
Activity
Created by: codecov[bot]
Codecov Report
Merging #13288 into main will decrease coverage by
3.26%
. The diff coverage is73.25%
.@@ Coverage Diff @@ ## main #13288 +/- ## ========================================== - Coverage 51.92% 48.65% -3.27% ========================================== Files 1492 1472 -20 Lines 83359 82637 -722 Branches 6876 6558 -318 ========================================== - Hits 43283 40210 -3073 - Misses 36454 38820 +2366 + Partials 3622 3607 -15
Flag Coverage Δ #go 52.72% <73.25%> (-0.03%)
#integration 29.16% <ø> (+0.03%)
#storybook 14.43% <ø> (ø)
#typescript 37.23% <ø> (-12.44%)
#unit 25.08% <ø> (-8.88%)
Impacted Files Coverage Δ cmd/frontend/graphqlbackend/campaigns.go 0.00% <ø> (ø)
enterprise/internal/campaigns/spec_migration.go 1.65% <0.00%> (ø)
internal/campaigns/types.go 27.23% <0.00%> (-0.12%)
enterprise/internal/campaigns/service.go 76.04% <50.00%> (-0.59%)
enterprise/internal/campaigns/syncer.go 77.38% <53.84%> (-0.14%)
...terprise/internal/campaigns/resolvers/changeset.go 73.68% <79.31%> (-0.15%)
enterprise/internal/campaigns/reconciler.go 59.70% <84.61%> (+0.51%)
enterprise/internal/campaigns/store_changesets.go 82.20% <89.47%> (-0.43%)
...prise/internal/campaigns/service_apply_campaign.go 84.46% <100.00%> (+1.60%)
shared/src/util/jsonc.ts 0.00% <0.00%> (-100.00%)
... and 129 more Created by: mrnugget
I don't think we need it, or did you have a specific use-case in mind?
Just to be safer when checking for title/body on the client side and maybe display a message: "Syncing..." or something. But I think if we just make the external ID look good standalone then we're fine.
Created by: mrnugget
I was first confused by it being published before being synced, but I guess it makes sense, at least wording-wise. That sadly brings us to the point of introducing another boolean flag in the db, but I didn't find too much of a way around that, unless we attach the changeset spec (instead of just 0), detect it's importing and see that reconciler state is processing or queued.
I'd love to get @LawnGnome and @chrispine's thought on that. I also couldn't see a way around it. And: I think there's another boolean coming, because we also need to close changesets in the background.
I don't think the booleans are bad, though. Since our changesets basically act like "jobs" that can be executed multiple times, we need switches to say which job should run. And those switches are really hard to fit into a single field, especially since some booleans/state fields reflect code host state, others reflect sourcegraph state, others reflect desired sourcegraph state, etc.
Created by: mrnugget
I do worry a bit about the potential combinatorial explosion of the various state fields we have, but it's not like trying to stuff them into one field is going to make that magically better. Complex things are complex.
Generally speaking, I think my reaction is pretty much the same as Erik's: this feels simpler, even with the extra field and boilerplate in some of the GraphQL resolver methods, so therefore it feels like the right path.
You managed to perfectly articulate my thinking here.
We're all now fully aware of what we're dealing with here and I think we should all keep an eye out to see if we can maybe find a better abstraction to manage these different state transitions of a changeset.
Sometimes I do wonder whether it was a mistake to merge "Changesets, a thing that can exist on a code host" and "Changeset, a thing that we 'run' in the background" into the same table, but I can't think of another solution that wouldn't be more complex.
What we should also be fully aware of is that we don't leak these abstractions through all the layers. One idea is that we push more logic from the resolver layer down into the "model" layer and I might give this a shot.
@chrispine I sent you an invite to walk you through this PR (which includes the whole call stack from request to reconciler).
I'm going to merge this PR now, because 24 hours later I'm still happy with it and none of you spotted obvious mistakes.
As always: nothing's set in stone and we can come up with a better way to do this, we should do it.