artin/sourcegraph
1
0
Fork 0
mirror of https://github.com/sourcegraph/sourcegraph.git synced 2026-02-06 18:51:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

a8n: Allow updating individually published Changesets (#8150)

Browse Source 
* a8n: Allow updating individually published Changesets

* Use status.Total instead of CompletedCount()

* Make partial-Campaign-update code a bit easier to read
This commit is contained in:
Thorsten Ball 2020-02-03 14:23:42 +01:00 committed by GitHub
parent d8ed447273
commit 33ecb8f76f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 116 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

57
enterprise/internal/a8n/service.go
View File

@ -733,19 +733,6 @@ func (s *Service) UpdateCampaign(ctx context.Context, args UpdateCampaignArgs) (
return campaign, nil, nil
}
changesetCreation, err := tx.GetLatestChangesetJobCreatedAt(ctx, campaign.ID)
if err != nil {
return nil, nil, errors.Wrap(err, "getting latest changesetjob creation time")
}
if changesetCreation.IsZero() {
// If the campaign hasn't been published yet, we can simply update the
// attributes because no ChangesetJobs have been created yet.
// If not all ChangesetJobs have been created yet, that means the Campaign itself
// hasn't been published yet, but only individual changesets. In that case, we don't
// support update yet. See: https://github.com/sourcegraph/sourcegraph/issues/7915
return campaign, nil, tx.UpdateCampaign(ctx, campaign)
}
status, err := tx.GetCampaignStatus(ctx, campaign.ID)
if err != nil {
return nil, nil, err
@ -754,8 +741,23 @@ func (s *Service) UpdateCampaign(ctx context.Context, args UpdateCampaignArgs) (
return nil, nil, ErrUpdateProcessingCampaign
}
// Fast path: if we don't update the CampaignPlan, we don't need to rewire
// ChangesetJobs, but only update name/description if they changed.
published, err := campaignPublished(ctx, tx, campaign.ID)
if err != nil {
return nil, nil, err
}
partiallyPublished := !published && status.Total != 0
if !published && !partiallyPublished {
// If the campaign hasn't been published yet and no Changesets have
// been individually published (through the `PublishChangeset`
// mutation), we can simply update the attributes on the Campaign
// because no ChangesetJobs have been created yet that need updating.
return campaign, nil, tx.UpdateCampaign(ctx, campaign)
}
// If we do have to update ChangesetJobs/Changesets, here's a fast path: if
// we don't update the CampaignPlan, we don't need to rewire ChangesetJobs,
// but only update name/description if they changed.
if !updatePlanID && updateAttributes {
return campaign, nil, tx.ResetChangesetJobs(ctx, campaign.ID)
}
@ -772,10 +774,15 @@ func (s *Service) UpdateCampaign(ctx context.Context, args UpdateCampaignArgs) (
}
}
for _, c := range diff.Create {
err := tx.CreateChangesetJob(ctx, c)
if err != nil {
return nil, nil, err
// When we're doing a partial update and only update the Changesets that
// have already been published, we don't want to create new ChangesetJobs,
// since they would be processed and publish the other Changesets.
if !partiallyPublished {
for _, c := range diff.Create {
err := tx.CreateChangesetJob(ctx, c)
if err != nil {
return nil, nil, err
}
}
}
@ -811,6 +818,18 @@ func (s *Service) UpdateCampaign(ctx context.Context, args UpdateCampaignArgs) (
return campaign, changesets, tx.UpdateCampaign(ctx, campaign)
}
// campaignPublished returns true if all ChangesetJobs have been created yet
// (they might still be processing).
func campaignPublished(ctx context.Context, store *Store, campaign int64) (bool, error) {
changesetCreation, err := store.GetLatestChangesetJobCreatedAt(ctx, campaign)
if err != nil {
return false, errors.Wrap(err, "getting latest changesetjob creation time")
}
// GetLatestChangesetJobCreatedAt returns a zero time.Time if not all
// ChangesetJobs have been created yet.
return !changesetCreation.IsZero(), nil
}
type campaignUpdateDiff struct {
Delete []*a8n.ChangesetJob
Update []*a8n.ChangesetJob

84
enterprise/internal/a8n/service_test.go
View File

@ -382,6 +382,10 @@ func TestService_UpdateCampaignWithNewCampaignPlanID(t *testing.T) {
// Repositories for which we had CampaignJobs attached to the old CampaignPlan
oldCampaignJobs repoNames
// Repositories for which the ChangesetJob/Changeset have been
// individually published while Campaign was in draft mode
individuallyPublished repoNames
updatePlan, updateName, updateDescription bool
newCampaignJobs []newCampaignJobSpec
@ -439,7 +443,7 @@ func TestService_UpdateCampaignWithNewCampaignPlanID(t *testing.T) {
wantModified: repoNames{"repo-0"},
},
{
name: "1 unmodified, 1 modified, 1 new changeset",
name: "1 detached, 1 unmodified, 1 modified, 1 new changeset",
updatePlan: true,
oldCampaignJobs: repoNames{"repo-0", "repo-1", "repo-2"},
newCampaignJobs: []newCampaignJobSpec{
@ -463,6 +467,48 @@ func TestService_UpdateCampaignWithNewCampaignPlanID(t *testing.T) {
{repo: "repo-3"},
},
},
{
name: "draft campaign, 1 published unmodified, 1 modified, 1 detached, 1 new changeset",
campaignIsDraft: true,
updatePlan: true,
oldCampaignJobs: repoNames{"repo-0", "repo-1", "repo-2"},
individuallyPublished: repoNames{"repo-0"},
newCampaignJobs: []newCampaignJobSpec{
{repo: "repo-0"},
{repo: "repo-1", modifiedDiff: true},
{repo: "repo-3"},
},
wantUnmodified: repoNames{"repo-0"},
},
{
name: "draft campaign, 1 published unmodified, 1 published modified, 1 detached, 1 new changeset",
campaignIsDraft: true,
updatePlan: true,
oldCampaignJobs: repoNames{"repo-0", "repo-1", "repo-2"},
individuallyPublished: repoNames{"repo-0", "repo-1"},
newCampaignJobs: []newCampaignJobSpec{
{repo: "repo-0"},
{repo: "repo-1", modifiedDiff: true},
{repo: "repo-3"},
},
wantUnmodified: repoNames{"repo-0"},
wantModified: repoNames{"repo-1"},
},
{
name: "draft campaign, 1 published unmodified, 1 published modified, 1 published detached, 1 new changeset",
campaignIsDraft: true,
updatePlan: true,
oldCampaignJobs: repoNames{"repo-0", "repo-1", "repo-2"},
individuallyPublished: repoNames{"repo-0", "repo-1", "repo-2"},
newCampaignJobs: []newCampaignJobSpec{
{repo: "repo-0"},
{repo: "repo-1", modifiedDiff: true},
{repo: "repo-3"},
},
wantUnmodified: repoNames{"repo-0"},
wantModified: repoNames{"repo-1"},
wantDetached: repoNames{"repo-2"},
},
}
for _, tt := range tests {
@ -516,6 +562,28 @@ func TestService_UpdateCampaignWithNewCampaignPlanID(t *testing.T) {
oldChangesets = fakeRunChangesetJobs(ctx, t, store, now, campaign, campaignJobsByID)
}
if tt.campaignIsDraft && len(tt.individuallyPublished) != 0 {
toPublish := make(map[int64]*a8n.CampaignJob)
for _, name := range tt.individuallyPublished {
repo, ok := reposByName[name]
if !ok {
t.Errorf("unrecognized repo name: %s", name)
}
for _, j := range oldCampaignJobs {
if j.RepoID == int32(repo.ID) {
toPublish[j.ID] = j
err = svc.CreateChangesetJobForCampaignJob(ctx, j.ID)
if err != nil {
t.Fatalf("Failed to individually created ChangesetJob: %s", err)
}
}
}
}
oldChangesets = fakeRunChangesetJobs(ctx, t, store, now, campaign, toPublish)
}
oldTime := now
now = now.Add(5 * time.Second)
@ -591,21 +659,25 @@ func TestService_UpdateCampaignWithNewCampaignPlanID(t *testing.T) {
// When a campaign is created as a draft, we don't create
// ChangesetJobs, which means we can return here after checking
// that we haven't created ChangesetJobs
if tt.campaignIsDraft {
if len(newChangesetJobs) != 0 {
t.Fatalf("changesetJobs created even though campaign is draft. have=%d", len(newChangesetJobs))
if tt.campaignIsDraft && len(tt.individuallyPublished) == 0 {
if have, want := len(newChangesetJobs), len(tt.individuallyPublished); have != want {
t.Fatalf("changesetJobs created even though campaign is draft. have=%d, want=%d", have, want)
}
return
}
var wantChangesetJobLen int
if tt.updatePlan {
wantChangesetJobLen = len(newCampaignJobs)
if len(tt.individuallyPublished) != 0 {
wantChangesetJobLen = len(tt.individuallyPublished)
} else {
wantChangesetJobLen = len(newCampaignJobs)
}
} else {
wantChangesetJobLen = len(oldCampaignJobs)
}
if len(newChangesetJobs) != wantChangesetJobLen {
t.Fatalf("wrong number of new ChangesetJobs. want=%d, have=%d", len(newCampaignJobs), wantChangesetJobLen)
t.Fatalf("wrong number of new ChangesetJobs. want=%d, have=%d", wantChangesetJobLen, len(newChangesetJobs))
}
newChangesetJobsByRepo := map[string]*a8n.ChangesetJob{}