Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

campaigns: allow the target branch to be customised #15070

Closed
LawnGnome opened this issue Oct 26, 2020 · 9 comments · Fixed by sourcegraph/src-cli#393
Closed

campaigns: allow the target branch to be customised #15070

LawnGnome opened this issue Oct 26, 2020 · 9 comments · Fixed by sourcegraph/src-cli#393
Assignees
Labels
batch-changes Issues related to Batch Changes estimate/1d good first issue Good for newcomers

Comments

@LawnGnome
Copy link
Contributor

A campaign always created a changeset against the repo's default branch at present, but there are scenarios where it may be desirable to allow customising the target branch, presumably through a new, optional changesetTemplate field.

@LawnGnome LawnGnome added good first issue Good for newcomers batch-changes Issues related to Batch Changes labels Oct 26, 2020
@LawnGnome LawnGnome added this to the Backlog milestone Oct 26, 2020
@eseliger
Copy link
Member

Another idea would be to leverage the already existing repositories field:

repositories:
  - github.com/sourcegraph/sourcegraph
  - name: github.com/sourcegraph/src-cli
    branch: release

That way, it would also support multi-root changeset creation (monorepo support)

repositories:
  - name: github.com/sourcegraph/src-cli
    workDirs:
      - cmd
      - internal

@bobheadxi
Copy link
Member

bobheadxi commented Oct 27, 2020

Feature request: let this base branch be created from the default branch if it does not yet exist

@chrispine
Copy link
Contributor

@chrispine
Copy link
Contributor

@eseliger Could you please add an estimate to this? Thanks!

@eseliger
Copy link
Member

Tasks I think need to be done for this one:

  • Come up with syntax and extend campaign spec schema
  • Add parsing to src-cli and validate given branches, like we do with defaultBranch currently
  • Make sure the reconciler takes the correct branch from the changeset spec

Overall doesn't sound too complicated and the UI already supports it, so I think the estimation of 1d should be fine.

@LawnGnome
Copy link
Contributor Author

@mrnugget
Copy link
Contributor

Just adding more info here:

on.repository already supports specifying the base branch from which the diff is created. See docs here

We don't use it though (which is a bug): https://github.com/sourcegraph/src-cli/blob/b36136bc281c21dd23a0fcd292449e4cdad09e1c/internal/campaigns/service.go#L408-L414

That means, if we support the branch in on.repository correctly and use it as the base branch for the diff and for the changeset template, the original problem of this ticket would already be solved, I think, because users could then specify per repository which branch they want.

Originally I thought about adding a baseBranch field to changesetTemplate, but that doesn't make a lot of sense, since it would then overwrite the baseRef we already have and the diff was not built on top of that baseBranch.

And I don't think that makes sense. What users most likely want is to say: "Do not use main as the base branch in this repository", which is what will work with the already existing syntax and if we make it work correctly.

If they ever want to say "use main/default-branch as the base branch for the diff, but open the changeset against another branch" then we need to come up with another field, like targetBranch or something. But I don't think that's what users right now want — any opinions on this?

@eseliger
Copy link
Member

Good find! I think we should go ahead and implement the behavior we documented already, it seems like an easy-to-understand solution that'll most likely help most customers for a while. We should just make sure in the code that this branch exists.

@mrnugget
Copy link
Contributor

I think we should go ahead and implement the behavior we documented already, it seems like an easy-to-understand solution that'll most likely help most customers for a while.

Agree. And if that wasn't clear: that was my proposal and what I aim to do :)

The other bit of good news is that if we handle the base branch correctly in on we can also maybe get things like repositoriesMatchingQuery: repo:sourcegraph/[email protected] foobar to work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
batch-changes Issues related to Batch Changes estimate/1d good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants