Skip to content

Re-attach changesets to campaign that were owned by campaign

Warren Gifford requested to merge mrn/reattach-detached-changesets into main

Created by: mrnugget

This fixes https://github.com/sourcegraph/sourcegraph/issues/13849 by re-opening changesets that were previously detached from the campaign.

See this Slack thread for a demo video that I recorded to show off this bug fix and what it fixes.

Thoughts on design

I'm not 100% happy with the code in the reconciler yet. I have the feeling that there's some design that makes the distinction between applyCampaign-directed state transitions (e.g. closing a changeset when detaching it from a campaign, or, like here, re-opening a detached one) and user-directed updates (e.g. update title and commit of a PR) clearer and easier to handle.

At its heart this problem revolves around the fact that a subset of the desired changeset state is included in the changeset spec and another set is not (since we don't allow closing in the spec). If we include our internal-state-directives in the changeset specs, they'd need to be in the schema and we'd need to create new changeset specs every time we want to make a change. That seems odd.

What we could do is to make this idea of internal-state-directives clearer by bundling them in a single column, for example, or treating all of them the same way. Not sure yet.

However, I think the code is fine to merge as it is. It's just something I keep thinking about.

Caveat

As it stands, this implementation could lead to changesets being re-opened after the user closed them on the code host. In practice, though, this won't currently happen, since the reconciler is only kicked off on user action, in which case we do want to reopen the changesets.

In order to avoid that, we could add another boolean on the changesets, reopening bool, that, just like closing, would cause the reconciler to reopen the changeset only on applyCampaign.

Merge request reports

Loading