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

Delete expired CampaignSpecs and ChangesetSpecs #12210

Merged
merged 5 commits into from
Jul 16, 2020

Conversation

mrnugget
Copy link
Contributor

This is the next item in the task list in https://github.com/sourcegraph/sourcegraph/pull/11675 and just simple housekeeping:

  • delete ChangesetSpecs if they haven't been attached to a CampaignSpec
  • delete CampaignSpecs if there is no Campaign with a reference to them (meaning that they haven't been applied yet)

@mrnugget mrnugget added campaigns batch-changes Issues related to Batch Changes labels Jul 15, 2020
@mrnugget mrnugget added this to the 3.18 milestone Jul 15, 2020
@mrnugget mrnugget requested review from a team July 15, 2020 15:32
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more box ticked off ✅ 🙂

enterprise/internal/campaigns/store_campaign_specs.go Outdated Show resolved Hide resolved
enterprise/cmd/repo-updater/main.go Show resolved Hide resolved
@mrnugget mrnugget mentioned this pull request Jul 15, 2020
28 tasks
Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have a few questions, but nothing blocking.

enterprise/internal/campaigns/store_campaign_specs_test.go Outdated Show resolved Hide resolved
enterprise/internal/campaigns/store_campaign_specs_test.go Outdated Show resolved Hide resolved
internal/campaigns/types.go Outdated Show resolved Hide resolved
@eseliger eseliger force-pushed the campaign-spec-api-v1 branch from c9be98a to 0867893 Compare July 15, 2020 23:32
@mrnugget mrnugget requested review from a team July 15, 2020 23:32
@eseliger eseliger force-pushed the delete-expired-specs branch from 4d2cdbe to 27a6890 Compare July 15, 2020 23:54
@eseliger eseliger removed request for a team, nicksnyder and slimsag July 15, 2020 23:56
@eseliger eseliger force-pushed the delete-expired-specs branch from 27a6890 to 28ca6ca Compare July 16, 2020 00:03
@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #12210 into campaign-spec-api-v1 will increase coverage by 3.89%.
The diff coverage is 60.00%.

@@                   Coverage Diff                    @@
##           campaign-spec-api-v1   #12210      +/-   ##
========================================================
+ Coverage                 46.92%   50.81%   +3.89%     
========================================================
  Files                       716     1419     +703     
  Lines                     21703    80287   +58584     
  Branches                   6574     6640      +66     
========================================================
+ Hits                      10184    40798   +30614     
- Misses                    11459    35947   +24488     
- Partials                     60     3542    +3482     
Flag Coverage Δ
#go 52.26% <60.00%> (?)
#integration 24.11% <ø> (-0.02%) ⬇️
#storybook 11.88% <ø> (ø)
#typescript 46.91% <ø> (-0.01%) ⬇️
#unit 47.50% <60.00%> (+12.83%) ⬆️
Impacted Files Coverage Δ
internal/campaigns/types.go 29.56% <0.00%> (ø)
...erprise/internal/campaigns/store_campaign_specs.go 81.56% <71.42%> (ø)
...rprise/internal/campaigns/store_changeset_specs.go 83.56% <71.42%> (ø)
...rise/internal/campaigns/resolvers/campaign_spec.go 70.96% <100.00%> (ø)
...ise/internal/campaigns/resolvers/changeset_spec.go 85.71% <100.00%> (ø)
web/src/settings/SettingsPage.tsx 64.70% <0.00%> (-11.77%) ⬇️
internal/vfsutil/archive.go 61.40% <0.00%> (ø)
...internal/codeintel/bundles/client/bundle_client.go 92.00% <0.00%> (ø)
...ecise-code-intel-indexer/internal/indexer/fetch.go 38.46% <0.00%> (ø)
cmd/frontend/internal/cli/serve_cmd.go 1.97% <0.00%> (ø)
... and 699 more

@mrnugget mrnugget merged commit f734211 into campaign-spec-api-v1 Jul 16, 2020
@mrnugget mrnugget deleted the delete-expired-specs branch July 16, 2020 08:52
eseliger added a commit that referenced this pull request Jul 16, 2020
* 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]>
mrnugget added a commit that referenced this pull request Jul 20, 2020
* 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]>
mrnugget added a commit that referenced this pull request Jul 22, 2020
* 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]>
mrnugget added a commit that referenced this pull request Jul 22, 2020
* 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]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants