batches: Change git diffstat to (added, deleted) from (added, modified, deleted) by BolajiOlajide · Pull Request #40454 · sourcegraph/sourcegraph-public-snapshot

BolajiOlajide · 2022-08-17T04:20:55Z

Test plan

I confirmed all diffs generated only display the added and deleted fields.

LawnGnome

I gave this a super quick once over after our discussion earlier at sprint planning, and this LGTM at a first pass. Looking forward to this being ready for review!

eseliger · 2022-09-08T19:49:35Z

Gave it a quick glance and got some thoughts:

We don't modify the global type DiffStat in GraphQL, which means that the changed property would still exist in the API. It now seems that out of all resolvers in Sourcegraph code batch changes resolvers seem to calculate that field wrong because every other API still returns it correctly. 🤔
I think it would be great to remove it from the global type as well, so that we don't end up with two DiffStat declarations and two separate concepts that users have to understand (in BC there are no changed lines, in everything else there are).
This also causes another problem: The fileDiffs use the global type, as it's a global resolver, and that causes the diffs themselves to still have changed lines, although the summary only knows added and deleted. That looks a bit confusing to me, even if we had to do two concepts for a while, at no time should both of them be visible on the same page:

This will also mean that batch changes will have to translate the DiffStat we use elsewhere in the codebase to our new approach.
I'm sorry I haven't been more clear in the ticket about the scope of this, but I think that we should do something that is consistent around all of the product.

One missed on the step resolver:

sourcegraph-bot · 2022-09-10T10:14:44Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 156b65a...a526094.

Notify	File(s)
@courier-new	client/web/src/enterprise/batches/batch-spec/batch-spec.mock.ts client/web/src/enterprise/batches/batch-spec/execute/backend.ts client/web/src/enterprise/batches/close/BatchChangeClosePage.story.tsx client/web/src/enterprise/batches/detail/BatchChangeDetailsPage.mock.ts client/web/src/enterprise/batches/detail/BatchChangeStatsCard.story.tsx client/web/src/enterprise/batches/detail/changesets/BatchChangeChangesets.mock.ts client/web/src/enterprise/batches/detail/changesets/ExternalChangesetNode.story.tsx client/web/src/enterprise/batches/preview/BatchChangePreviewPage.story.tsx client/web/src/enterprise/batches/preview/list/VisibleChangesetApplyPreviewNode.tsx client/web/src/enterprise/batches/preview/list/backend.ts client/web/src/enterprise/batches/preview/list/storyData.ts client/web/src/enterprise/batches/repo/BatchChangeRepoPage.story.tsx client/web/src/enterprise/batches/repo/backend.ts client/web/src/enterprise/batches/repo/testData.ts client/web/src/integration/batches.test.ts
@eseliger	client/web/src/enterprise/batches/batch-spec/batch-spec.mock.ts client/web/src/enterprise/batches/batch-spec/execute/backend.ts client/web/src/enterprise/batches/close/BatchChangeClosePage.story.tsx client/web/src/enterprise/batches/detail/BatchChangeDetailsPage.mock.ts client/web/src/enterprise/batches/detail/BatchChangeStatsCard.story.tsx client/web/src/enterprise/batches/detail/changesets/BatchChangeChangesets.mock.ts client/web/src/enterprise/batches/detail/changesets/ExternalChangesetNode.story.tsx client/web/src/enterprise/batches/preview/BatchChangePreviewPage.story.tsx client/web/src/enterprise/batches/preview/list/VisibleChangesetApplyPreviewNode.tsx client/web/src/enterprise/batches/preview/list/backend.ts client/web/src/enterprise/batches/preview/list/storyData.ts client/web/src/enterprise/batches/repo/BatchChangeRepoPage.story.tsx client/web/src/enterprise/batches/repo/backend.ts client/web/src/enterprise/batches/repo/testData.ts client/web/src/integration/batches.test.ts enterprise/internal/batches/service/service_test.go enterprise/internal/batches/state/state.go enterprise/internal/batches/store/batch_changes.go enterprise/internal/batches/store/batch_changes_test.go enterprise/internal/batches/store/batch_specs.go enterprise/internal/batches/store/batch_specs_test.go enterprise/internal/batches/store/changeset_specs.go enterprise/internal/batches/store/changeset_specs_test.go enterprise/internal/batches/store/changesets.go enterprise/internal/batches/store/changesets_test.go enterprise/internal/batches/testing/changeset.go enterprise/internal/batches/testing/changeset_spec.go enterprise/internal/batches/testing/mock_sync_state.go enterprise/internal/batches/testing/specs.go enterprise/internal/batches/types/changeset.go enterprise/internal/batches/types/changeset_spec.go enterprise/internal/batches/types/changeset_test.go internal/usagestats/batches.go internal/usagestats/batches_test.go

courier-new

Thanks for all your work on this, especially when it comes to the tedium of updating all the tests that broke. 😂 Looks great to me. Saw your updates in the analytics, repo, too -- appreciate that!

Co-authored-by: Randell Callahan <piszmogcode@gmail.com>

Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>

eseliger · 2022-09-19T15:32:17Z

+  {
+    "path": "enterprise/cmd/frontend/internal/batches/webhooks",
+    "prefix": "TestWebhooksIntegration",
+    "reason": "Removed the `diff_stat_changed` column from changeset and changeset_specs table."


I think the best way to do this next time would be to keep the column one more release but not use it - that way existing frontend pods won't error out before they get bumped for the upgrade and also the backcompat tests won't fail :)

Got it. thanks!

BolajiOlajide added the batch-changes Issues related to Batch Changes label Aug 17, 2022

BolajiOlajide assigned BolajiOlajide and adeola-ak Aug 17, 2022

cla-bot Bot added the cla-signed label Aug 17, 2022

BolajiOlajide marked this pull request as draft August 17, 2022 04:26

BolajiOlajide force-pushed the bo/change-diff-stat-values branch from ef51737 to 39c2cc6 Compare August 22, 2022 06:53

BolajiOlajide commented Aug 22, 2022

View reviewed changes

Comment thread internal/usagestats/batches.go

BolajiOlajide force-pushed the bo/change-diff-stat-values branch from c50920b to 3fc1f96 Compare September 3, 2022 03:50

LawnGnome reviewed Sep 8, 2022

View reviewed changes

Comment thread enterprise/cmd/frontend/internal/batches/resolvers/apitest/types.go Outdated

BolajiOlajide force-pushed the bo/change-diff-stat-values branch 2 times, most recently from 73bdf19 to 4a27ed8 Compare September 10, 2022 08:04

BolajiOlajide added the i-acknowledge-this-goes-into-4.0 label Sep 10, 2022

BolajiOlajide marked this pull request as ready for review September 10, 2022 10:13

BolajiOlajide requested a review from a team September 10, 2022 10:13

adeola-ak approved these changes Sep 11, 2022

View reviewed changes

Piszmog approved these changes Sep 12, 2022

View reviewed changes

Comment thread enterprise/internal/batches/state/state.go Outdated

BolajiOlajide force-pushed the bo/change-diff-stat-values branch 4 times, most recently from ade602c to ec7cb8d Compare September 15, 2022 02:10

courier-new approved these changes Sep 15, 2022

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread cmd/frontend/graphqlbackend/schema.graphql Outdated

Comment thread enterprise/internal/batches/state/state.go Outdated

Comment thread enterprise/internal/batches/store/batch_specs_test.go Outdated

BolajiOlajide added 7 commits September 15, 2022 20:46

update editorconfig to not insert new line for squashed.sql

6c3f4a0

create migrations

3724f57

add migration for updating reconciler changeset

fa67695

update migration

03fc130

remove diff_stat_changed

1f25d81

remove reference to DiffStatChanged

38afb1f

remove more references to changed

7f15469

BolajiOlajide and others added 25 commits September 15, 2022 20:47

update sql statements

cf57064

cleanup

ef502de

update pings

34df7d2

fix resolve tests

9d9b72b

fix resolver tests

790eeb8

update

7f988d5

remove changed fields

795e64f

rework diff logic

edc718b

add changelog record

44e438c

fix ts build errors

e323544

update snapshot for DiffStat

293c484

update database schema

aa64e26

make drop column migration idempotent

9ab6b7d

fix backend tests

fdc7b28

update changelog

3b7db07

prettier

efd8605

Update enterprise/internal/batches/state/state.go

85e227b

Co-authored-by: Randell Callahan <piszmogcode@gmail.com>

add backcompat file for breaking changes

269ca2f

correct typo in path

a90ac65

remove redundant changelog record

528b37d

cleanup

2310539

Update enterprise/internal/batches/state/state.go

39cd53f

Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>

Update CHANGELOG.md

6e82c37

Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>

add ExternalID to batch_spec_test

d13d23d

update docstring for DiffStat type

adce4c7

BolajiOlajide force-pushed the bo/change-diff-stat-values branch from d5f2e70 to adce4c7 Compare September 15, 2022 19:55

fix punctuation

a526094

BolajiOlajide merged commit d838320 into main Sep 19, 2022

BolajiOlajide deleted the bo/change-diff-stat-values branch September 19, 2022 11:57

eseliger reviewed Sep 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batches: Change git diffstat to (added, deleted) from (added, modified, deleted)#40454

batches: Change git diffstat to (added, deleted) from (added, modified, deleted)#40454
BolajiOlajide merged 33 commits into
mainfrom
bo/change-diff-stat-values

BolajiOlajide commented Aug 17, 2022 •

edited

Loading

Uh oh!

Uh oh!

LawnGnome left a comment

Uh oh!

Uh oh!

eseliger commented Sep 8, 2022

Uh oh!

sourcegraph-bot commented Sep 10, 2022 •

edited

Loading

Uh oh!

Uh oh!

courier-new left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eseliger Sep 19, 2022

Uh oh!

BolajiOlajide Sep 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

BolajiOlajide commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

Uh oh!

LawnGnome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eseliger commented Sep 8, 2022

Uh oh!

sourcegraph-bot commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

courier-new left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eseliger Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

BolajiOlajide Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

BolajiOlajide commented Aug 17, 2022 •

edited

Loading

sourcegraph-bot commented Sep 10, 2022 •

edited

Loading