Add a basic implementation of the applyCampaign mutation #11934

mrnugget · 2020-07-03T13:14:09Z

This is another PR to be merged into https://github.com/sourcegraph/sourcegraph/pull/11675 and adds a basic implementation of the applyCampaign mutation.

What the mutation already does:

Find/create a campaign based on the given namespace and the name of the given campaignSpec.
Transfer fields from the given campaignSpec to the campaign — I'm not sure whether we want to keep that. I can see some pros in copying the data, but I don't think it's necessary, since to compute the "desired state" we'll need to have the campaign, campaignSpec and changesetSpecs at hand anyway.
handle the ensureCampaign parameter

What I'm not sure about:

Copying fields, mentioned above.
why we need a namespace: ID! in applyCampaign, since the campaignSpec has a namespace already.

codecov · 2020-07-03T15:03:22Z

Codecov Report

Merging #11934 into campaign-spec-api-v1 will increase coverage by 0.03%.
The diff coverage is 75.00%.

@@                   Coverage Diff                    @@
##           campaign-spec-api-v1   #11934      +/-   ##
========================================================
+ Coverage                 47.41%   47.45%   +0.03%     
========================================================
  Files                      1396     1396              
  Lines                     79063    79163     +100     
  Branches                   6583     6679      +96     
========================================================
+ Hits                      37486    37563      +77     
- Misses                    38072    38083      +11     
- Partials                   3505     3517      +12

Flag	Coverage Δ
#go	`51.67% <75.00%> (+0.04%)`	⬆️
#storybook	`9.96% <ø> (ø)`
#typescript	`36.10% <ø> (ø)`
#unit	`47.04% <75.00%> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
internal/campaigns/types.go	`11.39% <ø> (ø)`
...nterprise/internal/campaigns/resolvers/resolver.go	`62.85% <57.57%> (-0.05%)`	⬇️
enterprise/internal/campaigns/service.go	`71.12% <80.39%> (+1.70%)`	⬆️
enterprise/internal/campaigns/store.go	`85.56% <90.00%> (+0.03%)`	⬆️
internal/authz/bitbucketserver/store.go	`67.74% <0.00%> (-0.93%)`	⬇️
.../internal/codeintel/resolvers/graphql/locations.go	`83.63% <0.00%> (ø)`
...terprise/internal/campaigns/resolvers/campaigns.go	`65.13% <0.00%> (+1.97%)`	⬆️

eseliger

Copying fields, mentioned above.

I don't see a problem here, it also simplifies the queries and makes it easier to look up things in the db.

why we need a namespace: ID! in applyCampaign, since the campaignSpec has a namespace already.

I wouldn't see why we need it. That would also make one argument less to the call that applies the spec from the src-cli.

eseliger · 2020-07-03T15:50:28Z

enterprise/internal/campaigns/store.go

 	}

 	if len(preds) == 0 {
 		preds = append(preds, sqlf.Sprintf("TRUE"))
 	}

-	return sqlf.Sprintf(getCampaignsQueryFmtstr, sqlf.Join(preds, "\n AND "))
+	var joinClause string
+	if opts.CampaignSpecName != "" {


should there be a protection for opts.CampaignSpecName != "" && opts.NamespaceUserID + opts.NamespaceOrgID == 0?

I see where you're coming from, but I don't think that's necessary. It would also kinda break the "API" of our store methods, which are very much: you need to know what you're doing :D

sqs · 2020-07-06T02:42:42Z

Merging because it was approved.

I will make a separate PR to address @mrnugget's point:

why we need a namespace: ID! in applyCampaign, since the campaignSpec has a namespace already.

(We don't need that, and I'll remove it in the separate PR. I forgot to remove it in the schema.graphql.)

* WIP: New campaigns workflow * Remove old UI for switch to new campaigns workflow (#11891) * Add a basic implementation of the applyCampaign mutation (#11934) * update campaigns docs to reflect new flow (#11972) * update GraphQL API to remove unneeded namespace param, add applyCampaign test (#11982) * add applyCampaign resolver test, temporarily un-implement createCampaign The createCampaign mutation is strictly less powerful than applyCampaign, and it adds needless complexity to implement createCampaign now, especially since we'll be changing the impl of applyCampaign a lot. So, let's remove the createCampaign impl for now. Then we also need to change the resolver test from testing createCampaign to testing applyCampaign. * remove unneeded namespace param from createCampaign and applyCampaign mutations The namespace is immutably stored in the campaign spec, which is also provided as a param. * Implement moveCampaign mutation (#12049) * Fix campaigns web code after rebase * Implement ChangesetSpec resolver and validation of specs (#12092) * Implement ChangesetSpec.Description in resolver and data layer * Add ChangesetSpec/CampaignSpec schema and Validate() method * Validate ChangesetSpec against schema on create * Validate CampaignSpec against schema * Allow YAML as CampaignSpec input (caveat: 'on' needs to be quoted) * Fix critical whitespace error * Fix CampaignSpec.ParsedInput response * Remove debug log statement * Remove TODO comment * Fix indentation in raw specs for testing * Run prettier Co-authored-by: Erik Seliger <[email protected]> * Use fork of ghodss/yaml to use 'on' as key in CampaignSpec (#12153) As we noticed last week: we can't use `on` as a key in a `CampaignSpec` because `yaml.YAMLToJSON` would convert that `on` into a `true` and that in turn would fail when validated. The solution here is to use a custom fork of `ghodss/yaml` that allows passing in a custom unmarshal-function which does _not_ do the conversion. The function we're passing in comes from the yaml.v3 library which changed its behavior when parsing boolean values (see ghodss/yaml#65). We lose the ability to use `YAMLToJSONStrict` since that only works with yaml.v2, but yaml.v3 already warns about duplicated keys and the JSON schema validation gives us enough of a safety net for the rest. * Remove duplication in ChangesetSpec/CampaignSpec.UnmarshalValidate (#12156) * Remove repositoryDiffs (#12205) * Remove openChangesets resolver (#12209) We don't need it for the new UI anymore, so reducing the headache of maintaining it. * Implement minimal resolvers to incorporate schema changes (#12215) * Use same changeset resolver for hidden and visible changesets (#12221) * Fix compilation after rebase * Delete expired CampaignSpecs and ChangesetSpecs (#12210) * Fix compilation after rebase * Delete expired Campaign/ChangesetSpecs * Return correct ExpiresAt in Campaign/ChangesetSpec resolvers * Clean up tests for spec expiry * Remove unneeded Truncate Co-authored-by: Erik Seliger <[email protected]> * Remove unused dependency (#12245) * Implement placeholders for status and preview API (#12226) * Remove store.GetRepoIDsForFailedChangesetJobs (#12323) This was a left-over from the last PR that removed some functionality. I know that there's a lot more code that's technically unused now, but this one stood out since it had the really specific usecase of loading error messages for a campaign. * Another set of small follow-ups after introducing ExternalChangesetState (#12330) * Add a ChangesetState.Valid() method * Rename test method to reflect name of method under test * Update naming and un-export methods * Implement missing bits in CampaignSpec/ChangesetSpec resolvers (#12247) * Fix intendation of query in test * Implement CampaignSpec.ViewerCanAdminister * Add safety-check to ChangesetSpecDescription.Diff method * Clean up repository permissions test * Implement rest of ChangesetSpec and CampaignSpec resolvers * Add options to CountChangesetSpecsOpts * Implement changesetSpecConnectionResolver * Reduce test data after reducing test cases * Change type checks on ChangesetSpecDescription * Query __typename in test to make sure we get the right response * Use same repo query pattern in ChangesetResolver as in changesetSpecResolver * Implement the after param for changesetSpecs connection * Check for permissions in applyCampaign and moveCampaign * Add a test for ChangesetSpec.Description.Diff * Remove isHidden filter * Do not yield ChangesetSpec if repo is (soft-)deleted * Fix rebase gone wrong * Check namespace perms in CreateCampaignSpec/MoveCampaign (#12362) (This is part #11675 and the new workflow) Before this, every user could create a campaign spec in any org or user namespace. And the same was true for moveCampaign: a user could move a campaign into namespace they wanted. This changes both implementations in the service layer to check for the permissions of the current user (saved in the ctx): 1. If it's a site admin, they can create/move things in any namespace. 2. If not and the target namespace is an org, we check for org membership. 3. If not and the target namespace is a user, we check that it's the same as the current user. * Make schema for env more strict to only accept strings (#12335) Environment variables can only be strings, so we shouldn't accept int, bool etc. * Drop old campaigns tables and add missing columns (#12388) * Drop old campaigns tables and add missing columns This drops the following tables: - changeset_jobs - patches - patch_sets Why drop them? Since we decided we're going to migrate existing campaigns into read-only versions and that we don't want to keep working state (changeset_jobs) around, it doesn't make sense to keep the tables. Why keep empty tables? So, this removes the tables and all the code that depends on them. It also does two other things: - Add a `changeset_spec_id` column to `changesets` (that's not yet read/written to in the `campaigns.Store`) - Add three `diff_stat_*` columns to `changeset_specs`, populate them when creating a new `ChangesetSpec` and reading/writing them in the `campaigns.Store`. Why? Because I don't want to write another migration that adds diff stats to changeset specs in case those were created between the introduction of changeset specs and us adding the fields. * Do not show changeset count in frontend * Add notice to WIP campaigns docs that they're WIP (#12393) * Campaign frontend cleanup work (#12256) Co-authored-by: Quinn Slack <[email protected]> Co-authored-by: Erik Seliger <[email protected]>

mrnugget added 2 commits July 3, 2020 11:53

Allow querying campaigns by namespace and campaignSpec name

ec44c1f

Add a basic implementation of the applyCampaign mutation

d184973

mrnugget added campaigns batch-changes Issues related to Batch Changes labels Jul 3, 2020

mrnugget added this to the 3.18 milestone Jul 3, 2020

mrnugget self-assigned this Jul 3, 2020

mrnugget mentioned this pull request Jul 3, 2020

Campaigns: 3.18 Tracking Issue #11523

Closed

28 tasks

mrnugget added 3 commits July 3, 2020 16:45

Accept ensureCampaign param in applyCampaign

f90ca3a

Change ApplyCampaign to accepts options struct

dcd80c9

Use a transaction in ApplyCampaign

9129512

mrnugget requested a review from a team July 3, 2020 14:58

mrnugget marked this pull request as ready for review July 3, 2020 14:58

eseliger approved these changes Jul 3, 2020

View reviewed changes

sqs merged this pull request into campaign-spec-api-v1 Jul 6, 2020

sqs deleted the campaigns-spec/apply-campaign branch July 6, 2020 02:43

sqs pushed a commit that referenced this pull request Jul 6, 2020

Add a basic implementation of the applyCampaign mutation (#11934)

92cbf31

mrnugget added a commit that referenced this pull request Jul 9, 2020

Add a basic implementation of the applyCampaign mutation (#11934)

b5772c1

eseliger pushed a commit that referenced this pull request Jul 15, 2020

Add a basic implementation of the applyCampaign mutation (#11934)

50bd380

eseliger pushed a commit that referenced this pull request Jul 16, 2020

Add a basic implementation of the applyCampaign mutation (#11934)

bbae166

mrnugget added a commit that referenced this pull request Jul 20, 2020

Add a basic implementation of the applyCampaign mutation (#11934)

d0de822

mrnugget added a commit that referenced this pull request Jul 22, 2020

Add a basic implementation of the applyCampaign mutation (#11934)

ed57d05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a basic implementation of the applyCampaign mutation #11934

Add a basic implementation of the applyCampaign mutation #11934

mrnugget commented Jul 3, 2020 •

edited

Loading

codecov bot commented Jul 3, 2020 •

edited

Loading

eseliger left a comment

eseliger Jul 3, 2020

mrnugget Jul 3, 2020

sqs commented Jul 6, 2020 •

edited

Loading

Add a basic implementation of the applyCampaign mutation #11934

Add a basic implementation of the applyCampaign mutation #11934

Conversation

mrnugget commented Jul 3, 2020 • edited Loading

codecov bot commented Jul 3, 2020 • edited Loading

Codecov Report

eseliger left a comment

Choose a reason for hiding this comment

eseliger Jul 3, 2020

Choose a reason for hiding this comment

mrnugget Jul 3, 2020

Choose a reason for hiding this comment

sqs commented Jul 6, 2020 • edited Loading

mrnugget commented Jul 3, 2020 •

edited

Loading

codecov bot commented Jul 3, 2020 •

edited

Loading

sqs commented Jul 6, 2020 •

edited

Loading