update docs for new campaign flow

[sqs]

I am pre-writing the docs so we have a concise description of the new flow from the user's POV.

Preview as rendered docs at:

• https://docs.sourcegraph.com/@campaigns-new-flow/user/campaigns
• https://docs.sourcegraph.com/@campaigns-new-flow/user/campaigns/hello_world_campaign
• https://docs.sourcegraph.com/@campaigns-new-flow/dev/campaigns_design

Figma mock (slightly behind the docs pages above): https://www.figma.com/file/kRf5i7xf1p4zs3OyX0DUVp/Campaigns

---

Docs that led to this change: RFC 157 and Campaigns CLI and naming/concepts idea. The proposals and motivations from those docs will be fully incorporated into this PR's .md doc changes.

[sqs]

This doc page is much easier to read at http://docs.sourcegraph.com/@campaigns-new-flow/user/campaigns than in this diff.

[sqs]

Need to clarify that YOU (the user) choose what type of changes to make, it’s not something automatic or (yet) built in

[mrnugget]

Like it so far! One note though on something that has been a source of confusion in the past too: we should settle on a terminology to describe the patch -> open changeset on code host.

There's a few different phrases we use around that and they're often used interchangeably:

• publishing a changeset vs. publishing a patch
• previewing a changeset vs. viewing the patches

[sqs]

I started on the wireframes for the new flow, but I got stuck on how to handle the fact that both editing name/description or patches can trigger an update to all changesets. Need to work on this some more, hopefully tomorrow morning PDT.

[sqs]

Ignore the rest of the GraphQL diff below here. It was just for the UI mockup, it's not an important part of the proposed API for the new campaigns flow.

[sqs]

@eseliger @mrnugget See updated GraphQL API: https://github.com/sourcegraph/sourcegraph/pull/10921/files?file-filters%5B%5D=.graphql#diff-4529437f5564f5e65e957821a46ff9c5R372.

[mrnugget]

I like the consistency of the updated API!

Further general thoughts:

1. I don't think "do we need to keep track of IDs on the client?" is a problem anymore.
2. Adding to my previous comment (https://github.com/sourcegraph/sourcegraph/pull/10921#discussion_r440854517) I'm worried that adding ChangesetSpecs as separate entities (addressable by ID) would not simplify things a lot on the backend. One idea I had was to have something like createChangeset(spec: ChangesetSpecInput): Changeset! but then have the reconciler only pick up the changeset as soon as it's attached to a campaign. The spec would be saved in a column on changeset. That would've been the changeset-equivalent to createCampaign(spec: CampaignSpecInput: changesets: [ID!]): Campaign. That would simplify things in the backend, I think. But feel free to ignore that.

[sqs]

@mrnugget:

I'm worried that adding ChangesetSpecs as separate entities (addressable by ID) would not simplify things a lot on the backend. One idea I had was to have something like createChangeset(spec: ChangesetSpecInput): Changeset! but then have the reconciler only pick up the changeset as soon as it's attached to a campaign. The spec would be saved in a column on changeset. That would've been the changeset-equivalent to createCampaign(spec: CampaignSpecInput: changesets: [ID!]): Campaign. That would simplify things in the backend, I think. But feel free to ignore that.

(I renamed {upload => create}ChangesetSpec.)

I don't like createChangeset(ChangesetSpecInput): Changeset because it violates the model of "specs need to be applied before they are used". It would create a 3rd "phase of an object": "just an unapplied spec", "applied and has a current state", and the new "just an unapplied spec but uses the type (Changeset) of an applied thing with current state".

Anyway, if this helps, in the backend, you can still create a changesets row when createChangesetSpec is called. Its spec column will be set but its status (or whatever) column will not be set.

[mrnugget]

because it violates the model of "specs need to be applied before they are used".

Got it! That's a good way of thinking about it (and, in hindsight, the obvious way 😉 )

[mrnugget]

@sqs okay, that sounds good!

---

I'm trying to keep track of what I'm stilling missing here and what we need to define. Since I found it helpful to summarise my comments above, let me add the list here too, even though it's duplicating some of the comments:

• How do we query the current reconciliation status of a campaign?
• How do we preview campaign updates?
• How do we publish single changesets? How do we publish all of them?
• How do we manually add changesets to a campaign?
• What's the schema for the specs sent up by src-cli?
  • What does the JSON/YAML schema for CampaignSpecs look like? i.e. what's the schema of spec in createCampaignSpec(spec: String!)?
  • What does the JSON/YAML schema for ChangesetSpecs look like? i.e. what's the schema of spec in createChangesetSpec(spec: String!)?
  • Does the CampaignSpec include the changesetTemplate or is that expanded client-side and turned into single ChangesetSpecs?

cc @eseliger

[eseliger]

From here:

Yes, I think we need to switch to spec-only publishing of changesets. We don't want to add an extra place where the user's desired state lives (i.e., by adding a publishChangeset mutation and flipping a bit on a changeset when they call that mutation).

That makes me wonder whether we should add a "close flag" to the spec as well. The desired state would be closed: true, so if we stick to this model strictly, I think we would need to do it as well.
However, I'm not super sure we want the UI to basically be read-only completely. Just to gradually roll-out a campaign by publishing one after the other, it feels quite burdensome to always pull out an editor, set a boolean flag on one of the changesets and submit it via src-cli. I do see that this is how Kubernetes works and we wanted to loosely inspire our API by that, but I am unsure whether it would make it harder to use.
When looking into the future, I would imagine there is server-side execution implemented and I can 1. create my manifest in the UI using monaco and 2. pick from a curated list of pre-baked campaigns to make my code a bit healthier. Given that, I would either need to create a file locally to track that, or have that file "tracked" somewhere on the Sourcegraph side and be able to edit it from the UI.
For Kubernetes, however, there is no such thing as "one single source of truth manifest" outside of the k8s api either. I can patch the manifest, without knowing the whole original file, I can edit it from another location and have an outdated manifest in my repo and so on. Given that, we can maybe pick this up as well. Update the spec in the DB from the UI by allowing "Publish" (for example) using a button, which then updates the spec. I can in return get the manifest by src campaign get <id> -o yaml to update my local (repo) state.

[sqs]

I removed all impl changes from this PR because they are in https://github.com/sourcegraph/sourcegraph/pull/11675. This PR now contains only docs changes and GraphQL API changes, to make it easier to review.

[sqs]

@eseliger:

That makes me wonder whether we should add a "close flag" to the spec as well. The desired state would be closed: true, so if we stick to this model strictly, I think we would need to do it as well.

No, I don't think this is necessary. The source of truth for "desire for a changeset to be closed" is the code host, not the campaign spec. (In contrast, the source of truth for "desire for a changeset to be published" is the campaign spec.)

If you "close" a changeset in a campaign, that should translate directly into closing the (eg) pull request on GitHub.

---

So, then, consider this case: You just closed (either on GitHub or via Sourcegraph, which are 100% equivalent) the campaign's pull request in repository a/b on branch foo---but the campaign spec still says "I want to propose a changeset on repository a/b on branch foo".

What happens? Well, here's what we want to happen: nothing. The campaign should respect that its changeset was closed.

• If the campaign spec's changesetTemplate changes, the closed PR should not be updated.
• If the campaign spec's command steps change, the branch should not be updated. (It might've been deleted when the PR was closed.)
• If the PR is reopened, then it should start being updated again by the campaign.

---

So, what if we implement a nice, multi-select batch operation UI for changesets, and then you select 500 changesets and click "Close 500 selected changesets"? That is a distributed operation, and it may be tempting to say "at least for implementation purposes, we should model that as though those changeset templates had closed: false". Maybe...but the user's intent is a one-time action, not to set a new invariant. Make sure that clicking "Close" on Sourcegraph for a changeset doesn't prevent you from reopening it on the code host (and doesn't undo it if you reopen it on the code host)!

---

("A-ha", you might say, "this is true--but if you close a campaign and want to close associated changesets, that is another distributed operation, and we should (for implementation purposes) represent that as though you set closed: true in your campaign spec." I don't think so. Suppose you closed a campaign that was only partially finished being published. You wouldn't want closing it to finish publishing all the changesets only to close them immediately. You would just want it to close all the changesets that it had opened. So, this "close changesets when closing campaign" should be handled specially.)

[sqs]

@eseliger @LawnGnome Check out my updates:

• Files changed:
  • schema.graphql types ChangesetSpec, ChangesetDescription, ExistingChangesetReference, GitBranchChangesetDescription, GitCommitDescription
  • CampaignSpec.schema.json for what the YAML campaign file would look like. (The simple example is in the Hello World Campaign doc page linked in this PR description.)
• https://github.com/sourcegraph/sourcegraph/pull/10921#issuecomment-653727228

Feedback appreciated! I am working down the checkboxes at https://github.com/sourcegraph/sourcegraph/pull/11675. (@mrnugget is on vacation.)

[mrnugget]

@eseliger @LawnGnome Check out my updates:

Replying here to both changes (CampaignSpec.schema.json and the proposed changes in schema.graphql):

If I understand the changes correctly, it would mean that a lot of the fields that are defined in campaign-spec.yml are validated client side with the CampaignSpec.schema.json, but what will be send up to the server is:

1. ChangesetSpecs with full ChangesetDescriptions that are the result of an evaluated CampaignSpec and represent the desired state for each changeset in each repository.
2. a CampaignSpec that is basically just a copy of the original campaign-spec.yml but doesn't contain any additional information, except the namespace.

Correct?

If so, that would be a change from what I've started to implement in https://github.com/sourcegraph/sourcegraph/pull/11675 — a slight change, since I've left that part under-defined on purpose. In the current implementation, the changesetTemplate is defined on the CampaignSpec (https://github.com/sourcegraph/sourcegraph/pull/11675/files#diff-fa5e1bd8d3af52b49e4ee71fd894780eR1523-R1527) and the ChangesetSpec (see ChangesetSpecFields here)) doesn't contain "title", "body", "commit message" etc. But that implementation also had a lot of questions marks left (manually tracking existing changesets, for example).

I think your proposal is a good step forward and answers multiple of my conceptual/graphql questions in https://github.com/sourcegraph/sourcegraph/pull/11675 but to clarify our shared understanding, I think we really need to answer these two questions in particular:

1. What is the JSON schema for campaignSpec in createCampaignSpec(namespace: ID!, changesetSpecs: [ID!]!, campaignSpec: String!)?
2. What is the JSON schema for spec in createChangesetSpec(changesetSpec: String!)?

I think the answer to (1) is the CampaignSpec.schema.json that you posted above (because we need the name and description for the campaign from that, plus we want to keep the original around).

And the answer to (2) is practically the ChangesetDescription.

What do you think?

---

(Implementation note: I'm slightly worried about all the "write" overhead to the database, since ChangesetsSpecs are immutable and they all contain a lot of duplicate information. So a user runs apply -f spec.yml multiple times, we'd create a ton of duplicate data in the database. It would make the code a lot simpler and easier to understand, since the desired state for each changeset is completely contained in the ChangesetSpec, but it's also data-heavy)

[sqs]

@mrnugget:

Here is my response based on my thinking. Feel free to challenge my assumptions if you have better ideas.

If I understand the changes correctly, it would mean that a lot of the fields that are defined in campaign-spec.yml are validated client side with the CampaignSpec.schema.json

Yes, validated and evaluated client-side. The src CLI will know how to evaluate the changesetTemplate.

what will be send up to the server is:

ChangesetSpecs with full ChangesetDescriptions that are the result of an evaluated CampaignSpec and represent the desired state for each changeset in each repository.

Yes.

a CampaignSpec that is basically just a copy of the original campaign-spec.yml but doesn't contain any additional information, except the namespace.

Yes, although it could be confusing to allow specifying the namespace in the YAML file and in a CLI flag. See kubernetes/kubernetes#7789 for discussion of how this works for kubectl (basically, the kubectl -n flag overrides the metadata.namespace field; there are some edge cases that don't apply to us). We don't need to allow specifying namespace in the YAML file; we could always just store it separately (always require src campaign apply -n, always require the applyCampaign(namespace: ID!, ...) namespace parameter, store it in a separate DB column, etc.). Your call, just make sure the precedence is consistent and documented.

If so, that would be a change from what I've started to implement in #11675 — a slight change, since I've left that part under-defined on purpose. In the current implementation, the changesetTemplate is defined on the CampaignSpec (#11675 (files)) and the ChangesetSpec (see ChangesetSpecFields here)) doesn't contain "title", "body", "commit message" etc. But that implementation also had a lot of questions marks left (manually tracking existing changesets, for example).

Yes. I think it's good to make the ChangesetSpecs comprehensive. This will increase their expressive power. Right now, we don't have a way to have different changeset titles, descriptions, commit messages, etc., among the various changesets in a campaign, but I can certainly see that being useful. (Think of a lint campaign where the changesets say which lint problems were actually found, or where there are different manual steps to revoke different types of tokens leaked in code in addition to just removing them.)

I think your proposal is a good step forward and answers multiple of my conceptual/graphql questions in #11675 but to clarify our shared understanding, I think we really need to answer these two questions in particular:

(1) What is the JSON schema for campaignSpec in createCampaignSpec(namespace: ID!, changesetSpecs: [ID!]!, campaignSpec: String!)?
I think the answer to (1) is the CampaignSpec.schema.json that you posted above (because we need the name and description for the campaign from that, plus we want to keep the original around).

Yes.

(2) What is the JSON schema for spec in createChangesetSpec(changesetSpec: String!)?
The ChangesetDescription?

Yes.

[mrnugget]

I just pushed c346293, which adds a baseRev (not baseRef! which we already have) field to GitBranchChangesetDescription. Strictly speaking we don't need it on the read-end of the API, but since the src-cli needs to send it up and we will have the value at hand I thought I'd add it for completeness sake, consistency and for documentation purposes.

[eseliger]

See slack for storybook access.

After yesterdays call, I've filled the properties of the CampaignDelta type and added some storybooks for it, to showcase how it could be used.
See this commit for details:
https://github.com/sourcegraph/sourcegraph/pull/12047/commits/44bb1e1e8ec15dee8177971e1787613ff8fafc47
Which landed here in this storybook: https://5f078f888555130022c12385-qnepitfatb.chromatic.com/?path=/story/web-campaignupdatediff--campaigndelta-not-empty
The storybook doesn't reflect a final UI, but rather how useful it could be to have a changeset connection and augment the edges with the update information, so we can still have a single list view with all the changesets in one list. Note that it would require that we always have a Changeset for each spec, even if not applied yet. I think that is doable though and might save us one DB table.

---

The branch also includes changeset state updates, but don't look at them yet, they were based on Adams state diagram. I've created a separate one here: https://docs.google.com/drawings/d/1hicy46Cct9bU3DYdy-uskp1lxQ0KLj5rYdHBYVp7hh8/edit, please take a look there and once we are happy with how it turned out, I'll update the schema and the storybook page here: https://5f078f888555130022c12385-qnepitfatb.chromatic.com/?path=/story/web-changesetnode--visible-changeset

Work items

• Review state diagram and add comments
• Review Delta API spec (not storybook, it's just for visualization for now)

[mrnugget]

@LawnGnome @sqs I just discussed Erik's proposed schema (https://github.com/sourcegraph/sourcegraph/commit/44bb1e1e8ec15dee8177971e1787613ff8fafc47) and state diagram on a Zoom call.

Can you both also take a look and see if you can spot something that's wrong/could be improved?

The one big thought that just came up in our discussion was: should we create "empty" changesets when we create a changesetSpec (or: do we use a single table for both)? The reasoning behind this question is that the preview-delta-API as proposed by Erik above used changesets as the unit of change ("these 5 changesets will be added/closed/updated"). That works well when the changesets already exist (after the user applied a campaignSpec once, for example). But if you preview a campaignSpec that has never been applied, the delta would need to give show up "these 5 changesets will be created" and since they don't exist yet, they don't have an ID yet, etc. etc.

[mrnugget]

I thought about this ☝️ some more.

The two biggest question marks we have left in this new flow are:

1. API with which to preview applying of a campaign spec
2. API with which to get the current status of creation/updating/closing/syncing/... of a campaign and its changesets

Erik's proposal above combines them, by re-using the preview query to get the current status of the campaign. After thinking about this since Friday, I don't think that's a good idea. It mixes two previously unrelated concepts ("status" and "preview") with the hope of getting making things simpler by making them conform to a single concept. The result is complex, though, and would require us to bend over backwards to fit the preview API into the concept of a status API (by requiring us to create changesets for changeset specs that haven't been applied yet, for example).

Instead I think we should take Erik's proposed status API (which, in fact, is more a storybook right now) which would allow API consumers to get the "status" of a campaign by querying its changesets and fully design that.

And then we should design a separate campaignDelta API that's based on Quinn's proposed query (campaignDelta(namespace: ID!, campaignSpec: ID!, ensureCampaign: ID): CampaignDelta!) and that does not require us to create changesets even though nothing been applied yet. That might mean that we need some nullable fields in that part of the API, but I'm okay with that and I think we're often too scared of the null-boogeyman.

Let's discuss who's going to own the design of these two parts of the API on today's sync, because I think we need more and clearer ownership here :)

[sqs]

Regarding CampaignDelta (requested by @mrnugget):

• This is a diff, not a "preview/plan of what steps will be taken to achieve a desired state". Because this is a distributed system, any steps produced would be immediately out of date (because things can change at any moment in the external systems that make up the overall state of the system). This is mostly an issue of naming. We will still have a UI that implies "when you apply this campaign spec, these are the changes that will happen". But for the GraphQL API naming, let's avoid usage of terms that could imply that this is a definite preview/plan.
• Should the CampaignDelta compare the current spec or the current status (to the new spec)? I don't know.
• We want the UI to be able to show a granular diff of (1) the changes to the campaign itself (which really just means the title, description, and maybe the namespace if that is a CampaignSpec field (see https://github.com/sourcegraph/sourcegraph/pull/10921#issuecomment-656272372)), and (2) the changes to the changesets, including:
  • For added changesets (that are in the new campaign spec and are not in the current one): showing their addition and what will happen (for example, PR will be opened vs. this is just an unpublished changeset),
  • For changed changesets: a granular list of the fields that change, such as PR title, description, diff, commits (each message and author). For v0, we can just indicate which fields changed and show both before/after instead of needing to show a super granular diff (such as "commit 3's author name changed").
  • For removed changesets (that are NOT in the new campaign spec and ARE in the current one): showing their removal and what will happen (for example, PR will be closed vs. just an unpublished changeset so no operations need to be performed on the code host).
• I actually think (with certainty 85%) that given the new explicit nature of the changeset specs, which make it really clear what the desired state is, we can postpone the CampaignDelta and it isn't a requirement to graduate from beta. We do still need a way to visually see a single campaign spec and all of its changesets, but we don't need to diff it. The user flow for creating a campaign will be nice and visual; for updating a campaign, they will just need to trust that we'll handle the delta-update correctly.

[mrnugget]

• Should the CampaignDelta compare the current spec or the current status (to the new spec)? I don't know.

I think it should compare current status to the new spec. Why? Because we're not 100% declarative, meaning that the state of a changeset can change on a code host (from open to closed, for example) and we won't try to re-open it.

• I actually think (with certainty 85%) that given the new explicit nature of the changeset specs, which make it really clear what the desired state is, we can postpone the CampaignDelta and it isn't a requirement to graduate from beta. We do still need a way to visually see a single campaign spec and all of its changesets, but we don't need to diff it. The user flow for creating a campaign will be nice and visual; for updating a campaign, they will just need to trust that we'll handle the delta-update correctly.

Can you expand on that?

Do you think a first solution that has "a way to visually see a single campaign spec and all of its changesets" would just be that: the CampaignSpec.PreviewURL leads you to a page where the client queries the given campaign spec and displays its changesets?

If that is acceptable as a first solution, I'd prefer to doing just that for now and defer the CampaignDelta until we have the rest working.

[mrnugget]

@sqs Did you have any ideas as to what the "preview" page for a campaign spec would look like?

Now that we said we don't show a delta view there, I suppose it's a list of the CampaignSpec and its ChangesetSpecs. But when talking to @eseliger we realised that there might be different ideas here.

Erik, for example, thought along the lines of "preview page is a variation of the campaign page".

I think it should be a standalone page that looks decidedly different in its layout (so we don't get into the problem that we had with the CampaignDetails view where it was used to display a patchset and patches, a campaign and changesets, a campaign with a patchset and patches and changesets, etc.).

And since we want to eventually implement a delta view, I assume, I don't think we should invest too much into the preview page. On the other hand, though, it's likely the first "campaigns" page that customers will see with the new flow.

[sqs]

@mrnugget:

Did you have any ideas as to what the "preview" page for a campaign spec would look like? > Now that we said we don't show a delta view there, I suppose it's a list of the CampaignSpec and its ChangesetSpecs. But when talking to @eseliger we realised that there might be different ideas here.

Erik, for example, thought along the lines of "preview page is a variation of the campaign page".
I think it should be a standalone page that looks decidedly different in its layout.

I agree it should look very different in its layout, kind of like how the Amazon checkout flow cuts out a lot of the noisy nav junk. I think it should have no search bar (GlobalNavbar variant low-profile-with-logo), no breadcrumbs, just the full campaign spec and changesets with a create-campaign button.

I also think it should show all diffs and data, without the need to selectively expand/collapse.

And since we want to eventually implement a delta view, I assume, I don't think we should invest too much into the preview page. On the other hand, though, it's likely the first "campaigns" page that customers will see with the new flow.

I think many of the components will be reused between the preview page today, the campaign view page today, and the future delta view. So I don't think this is a dichotomy. The preview page today should look and feel really nice, but I don't think that will take a ton of work. Making it simple (as mentioned above) will go a long way to making it look nice. Another way in which it'll be simpler than in the past is that there will just be changesets, not patches+changesets.