Gerrit Batch Changes by varsanojidan · Pull Request #52647 · sourcegraph/sourcegraph-public-snapshot

varsanojidan · 2023-05-30T19:54:15Z

Closes https://github.com/sourcegraph/sourcegraph/issues/50867
Closes https://github.com/sourcegraph/sourcegraph/issues/51929

📺 Loom: https://www.loom.com/share/31d7bdace69d4a5ab8c732339f0a4950

I decided to just close the Reviews PR, and open this up instead because as I was connecting everything I noticed quite a few things that weren't right and I thought it would make more sense to just merge the thing that works directly instead of making it a 2-step process.

This includes basically everything needed for Gerrit Batch Changes, but not webhooks (we don't know if that will be supported yet).

Test plan

Added tests

varsanojidan · 2023-05-30T20:37:43Z


+	// Gerrit publishes the Change at push time, and therefore
+	if e.ch.ExternalServiceType == extsvc.TypeGerrit {
+		exists = false


Wanted to call special attention to this "workaround" here, the Changeset "exists" at this point because it gets created at push time, therefore exists would always return true, and we would always enquere a ChangesetUpdate webhook event instead of the regular publish event.

TBH I'm not sure what ramifications this workaround may cause and wanted to point it out, because as far as I can tell everything works well.

could we move this into the changeset layer and return false for the exists flag from CreateChangeset / CreateDraftChangeset instead? That way, this loop would stay free of implementation details. Also, the description in this comment is clearer to me than the code comment so maybe copy that into the code as well? :)

sourcegraph-bot · 2023-05-30T20:41:34Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff c8f9efa...ab79214.

Notify	File(s)
@BolajiOlajide	client/web/src/enterprise/batches/settings/AddCredentialModal.tsx
@courier-new	client/web/src/enterprise/batches/settings/AddCredentialModal.tsx
@eseliger	client/web/src/enterprise/batches/settings/AddCredentialModal.tsx enterprise/internal/batches/reconciler/BUILD.bazel enterprise/internal/batches/reconciler/executor.go enterprise/internal/batches/sources/BUILD.bazel enterprise/internal/batches/sources/azuredevops.go enterprise/internal/batches/sources/bitbucketcloud.go enterprise/internal/batches/sources/bitbucketserver.go enterprise/internal/batches/sources/common.go enterprise/internal/batches/sources/gerrit.go enterprise/internal/batches/sources/gerrit/types.go enterprise/internal/batches/sources/gerrit_test.go enterprise/internal/batches/sources/github.go enterprise/internal/batches/sources/gitlab.go enterprise/internal/batches/sources/mocks_test.go enterprise/internal/batches/sources/testing/fake.go enterprise/internal/batches/state/changeset_history.go enterprise/internal/batches/state/state.go enterprise/internal/batches/store/BUILD.bazel enterprise/internal/batches/store/changesets.go enterprise/internal/batches/types/BUILD.bazel enterprise/internal/batches/types/changeset.go enterprise/internal/batches/types/changeset_event.go enterprise/internal/batches/types/changeset_event_test.go enterprise/internal/batches/types/changeset_test.go enterprise/internal/batches/webhooks/changeset.go internal/extsvc/gerrit/changes.go internal/extsvc/gerrit/changes_test.go internal/extsvc/gerrit/client.go internal/extsvc/gerrit/testdata/golden/GetChangeReviews.json internal/extsvc/gerrit/testdata/vcr/GetChangeReviews.yaml internal/extsvc/gerrit/testdata/vcr/SetReadyForReview.yaml internal/extsvc/gerrit/testdata/vcr/SetWIP.yaml internal/extsvc/gerrit/types.go
@indradhanush	cmd/gitserver/server/patch.go
@sashaostrikov	cmd/gitserver/server/patch.go internal/extsvc/gerrit/changes.go internal/extsvc/gerrit/changes_test.go internal/extsvc/gerrit/client.go internal/extsvc/gerrit/testdata/golden/GetChangeReviews.json internal/extsvc/gerrit/testdata/vcr/GetChangeReviews.yaml internal/extsvc/gerrit/testdata/vcr/SetReadyForReview.yaml internal/extsvc/gerrit/testdata/vcr/SetWIP.yaml internal/extsvc/gerrit/types.go
@sourcegraph/delivery	doc/admin/external_service/index.md
@unknwon	enterprise/internal/authz/gerrit/mocks_test.go

varsanojidan · 2023-05-30T22:57:28Z

I ran through all of the scenarios here, and all of them seem good (with the exception of forks because Gerrit doesn't support it).

eseliger

I ran through all of the scenarios https://github.com/sourcegraph/sourcegraph/pull/47913#issuecomment-1439155858, and all of them seem good (with the exception of forks because Gerrit doesn't support it).

If forks aren't supported, what happens when the flag is set? Does it error, or just ignore the flag?

eseliger · 2023-06-01T10:53:02Z


+	// Gerrit publishes the Change at push time, and therefore
+	if e.ch.ExternalServiceType == extsvc.TypeGerrit {
+		exists = false


could we move this into the changeset layer and return false for the exists flag from CreateChangeset / CreateDraftChangeset instead? That way, this loop would stay free of implementation details. Also, the description in this comment is clearer to me than the code comment so maybe copy that into the code as well? :)

varsanojidan · 2023-06-01T15:10:48Z

could we move this into the changeset layer and return false for the exists flag from CreateChangeset / CreateDraftChangeset instead? That way, this loop would stay free of implementation details. Also, the description in this comment is clearer to me than the code comment so maybe copy that into the code as well? :)

I think my brain must have lagged and not actually finished the comment lol, yeah I'll update it. I can give it a shot for sure, the reason I was hesitant was because the Change technically exists at the time you call CreateChangeset, so I wasn't sure if it might mess with some other logic, but I'll try it out and validate that everything behaves as expected.

sourcegraph-buildkite · 2023-06-01T16:41:20Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (0.00 kb)	-0.00% (-0.31 kb)	-0.00% (-0.31 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 2d3021b and aa7cc2e or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

sourcegraph-bot · 2023-06-01T16:42:40Z

📖 Storybook live preview

courier-new

👍👍👍👍
Thank you for adding all the tests here too!!!

varsanojidan · 2023-06-02T18:58:19Z

@courier-new thank you for the amazing feedback, I believe I should've addressed all of your suggestions in my most recent commit.

courier-new

Great changes, left a couple more minor comments.

Another question, since you didn't go this far in your Loom recording: What is the behavior for updating a changeset, like when you go back and execute an updated batch spec that changes the title or produces a different diff and you need to commit and push the new changes? My impression is that it would create a brand new Change on Gerrit, since GenerateGerritChangeID would produce a different ID now. Do we update the tracking okay, i.e. is there still only one Change associated with the batch change after that update? Do we do anything with the old Change on Gerrit/should we?

…gerrit-bc

varsanojidan · 2023-06-05T01:02:58Z

@courier-new I believe I should have addressed all of the comments, the last thing I need to do is validate the scenario when updating the Changeset now that we use the deterministic Change ID approach. I'll update here tomorrow when I look into it.

varsanojidan · 2023-06-05T15:22:57Z

@courier-new now that we do the deterministic ChangeID, updating the Batch Change (title and script), as you predicted, does create a new Change on the Gerrit code host, but the tracking is not updated locally, so the Batch Change still has the old Change.

As you called out, the creation of the new Change is happening because of the deterministic ChangeID generation. My question is, what behavior do we want here? Are we ok with it just creating a new change and we update the Changeset to point to the new Change? Do we want it to still be attached to the old change in which case the deterministic Change ID might be an issue here? I ask because I see for other code hosts we just update the existing PR.

varsanojidan · 2023-06-05T15:45:01Z

@courier-new I got it to work almost as expected here, it just required messing around a bit with Gerrit's BuildCommitOpts func to use an existing ChangeID instead of generating one if ti exists 😄.

…gerrit-bc

Closes https://github.com/sourcegraph/sourcegraph/issues/50867 📺 Loom: https://www.loom.com/share/31d7bdace69d4a5ab8c732339f0a4950 I decided to just close the [Reviews PR](https://github.com/sourcegraph/sourcegraph/pull/52470), and open this up instead because as I was connecting everything I noticed quite a few things that weren't right and I thought it would make more sense to just merge the thing that works directly instead of making it a 2-step process. This includes basically everything needed for Gerrit Batch Changes, but not webhooks (we don't know if that will be supported yet). ## Test plan Added tests

varsanojidan added 3 commits May 25, 2023 15:54

add reviews to Gerrit BC

87f0974

initial commit, gerrit bc support

e57607c

fixing a bunch of broken things

6a2ad84

cla-bot Bot added the cla-signed label May 30, 2023

test fix

330db1b

varsanojidan changed the title ~~Iv/gitserver support gerrit bc~~ Gerrit Batch Changes May 30, 2023

fix tests

f88dbb9

varsanojidan commented May 30, 2023

View reviewed changes

varsanojidan requested review from a team May 30, 2023 20:39

varsanojidan added team/repo-management gerrit-bc Gerrit Batch Changes work labels May 30, 2023

varsanojidan marked this pull request as ready for review May 30, 2023 20:39

varsanojidan commented May 30, 2023

View reviewed changes

Comment thread enterprise/internal/batches/reconciler/executor.go Outdated

varsanojidan added 2 commits May 30, 2023 18:24

docs

3981b75

fix issue when usin non-main branch

7d59d0c

bazel

f3939d0

eseliger reviewed Jun 1, 2023

View reviewed changes

address PR comments

63b3cc3

varsanojidan added 3 commits June 2, 2023 12:30

remove reqs from modal

5db3b84

prettier

c3aa502

self-close

2d3021b

courier-new approved these changes Jun 2, 2023

View reviewed changes

address PR feedback

a5c1eb8

varsanojidan added 2 commits June 2, 2023 15:01

Merge branch 'main' into iv/gitserver-support-gerrit-bc

b9b24b4

fix build

58be12e

courier-new reviewed Jun 4, 2023

View reviewed changes

varsanojidan added 11 commits June 4, 2023 13:14

address PR comments, and build errors

7b24fa8

resolve Merge conflicts

23f182d

change it back

14d4ada

addressing more PR comments

0c99741

cleaning up the pushing

ba91cf4

resolve build issues

e08baf5

license

0aa6d93

Merge remote-tracking branch 'origin/main' into iv/gitserver-support-…

a539474

…gerrit-bc

fix test

38903bb

undo unintended pnpm lock change

4ce80c9

remove newline

d635b87

varsanojidan added 2 commits June 5, 2023 14:02

address BC updates

12cb40c

Merge remote-tracking branch 'origin/main' into iv/gitserver-support-…

56d65b8

…gerrit-bc

varsanojidan enabled auto-merge (squash) June 5, 2023 18:30

pipeline issues

ab79214

varsanojidan merged commit be61c04 into main Jun 5, 2023

varsanojidan deleted the iv/gitserver-support-gerrit-bc branch June 5, 2023 18:51

Conversation

varsanojidan commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

varsanojidan May 30, 2023

Choose a reason for hiding this comment

Uh oh!

eseliger Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

sourcegraph-bot commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

varsanojidan commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eseliger left a comment

Choose a reason for hiding this comment

Uh oh!

eseliger Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

varsanojidan commented Jun 1, 2023

Uh oh!

sourcegraph-buildkite commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

sourcegraph-bot commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

varsanojidan commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

varsanojidan commented Jun 5, 2023

Uh oh!

varsanojidan commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varsanojidan commented Jun 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

varsanojidan commented May 30, 2023 •

edited

Loading

sourcegraph-bot commented May 30, 2023 •

edited

Loading

varsanojidan commented May 30, 2023 •

edited

Loading

sourcegraph-buildkite commented Jun 1, 2023 •

edited

Loading

sourcegraph-bot commented Jun 1, 2023 •

edited

Loading

varsanojidan commented Jun 2, 2023 •

edited

Loading

varsanojidan commented Jun 5, 2023 •

edited

Loading