batches: construct webhook payload via GraphQL request by BolajiOlajide · Pull Request #46967 · sourcegraph/sourcegraph-public-snapshot

BolajiOlajide · 2023-01-26T00:21:41Z

Test plan

Tested locally using webhook.site, and it looks good.

batch_change_apply
changeset_publish

sourcegraph-bot · 2023-01-26T00:23:45Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 7e39581...9e78379.

Notify	File(s)
@eseliger	enterprise/internal/batches/processor/bulk_processor.go enterprise/internal/batches/reconciler/executor.go enterprise/internal/batches/service/service.go enterprise/internal/batches/service/service_apply_batch_change.go enterprise/internal/batches/webhooks/batch_change.go enterprise/internal/batches/webhooks/batch_change_test.go enterprise/internal/batches/webhooks/changeset.go enterprise/internal/batches/webhooks/changeset_test.go enterprise/internal/batches/webhooks/gql.go enterprise/internal/batches/webhooks/util.go enterprise/internal/batches/webhooks/webhooks.go

eseliger

nice, feels super clean :)

courier-new · 2023-01-26T08:17:22Z

Hmm, y'know, this is cool and all, but I admit when Erik first suggested the idea, I guess imagined this looking a bit different. 😅 I was kinda hoping that would mean a net reduction of lines of code, maybe that was naive of me... 😂 It's also kinda unfortunate now that there's three places you would have to update the next time we add another field on a batch change.

But I'd love to understand better! So the main benefit we're getting here is that we no longer have to duplicate the namespace marshaling logic or any of the other little things to marshal values into JSON-friendly format? Are there other benefits? 🙂

courier-new · 2023-01-26T08:18:52Z

 	SyncerError         *string      `json:"syncer_error"`
 }

-func MarshalChangeset(ctx context.Context, db basestore.ShareableStore, cs *types.Changeset) ([]byte, error) {


Am I missing something here? It looks like MarshalChangeset is still just creating the changeset body payload manually, not with an internal GraphQL request.

@BolajiOlajide would we be able to do the same internal GraphQL query thing for MarshalChangeset, too?

Yes, I can do that.

How about another approach? Since we can depend on the GraphQL schema to construct the webhook payload, how about we change the signature of the MarshalX method to take in a graphql.ID as the last parameter instead of the full struct. That way func MarshalChangeset(ctx context.Context, cs *types.Changeset) ([]byte, error) becomes func MarshalChangeset(ctx context.Context, id graphql.ID) ([]byte, error) {

What do you think @courier-new @eseliger ?

I think we talked about this in Barcelona but just wanted to put it on the record here, too -- that sounds good to me!

eseliger · 2023-01-26T13:52:20Z

But I'd love to understand better! So the main benefit we're getting here is that we no longer have to duplicate the namespace marshaling logic or any of the other little things to marshal values into JSON-friendly format? Are there other benefits? 🙂

I think it has a bunch of benefits actually IMO:

No duplication of computed field values if the computation lives in the graphql layer
No divergence between webhook and graphql response possible
We get permission checks "for free" because the graphql request honors the user account in the context
No change of payload if the type changes, making it more stable (yes we have to add it here explicitly, but that actually feels very intentional so I don't dislike that)

How do you feel about that?

Piszmog · 2023-01-26T16:41:16Z

Could you elaborate a bit more on,

We get permission checks "for free" because the graphql request honors the user account in the context

I thought webhooks are configured with just a secret at the site admin level.

eseliger · 2023-01-27T14:19:30Z

I thought webhooks are configured with just a secret at the site admin level.

Ah, I guess that doesn't give us a lot of benefit then today :D when we start allowing webhooks on the user-level as well (site-admin is pretty clunky imo and puts a lot of config burden on often a single individual) it'll help 🤷

BolajiOlajide · 2023-01-27T18:12:02Z

I thought webhooks are configured with just a secret at the site admin level.

Ah, I guess that doesn't give us a lot of benefit then today :D when we start allowing webhooks on the user-level as well (site-admin is pretty clunky imo and puts a lot of config burden on often a single individual) it'll help 🤷

Yeah, I misunderstood the secrets aspect, also. Let's close this Pull Request and the associated issue.
One of the reasons I agreed with this approach initially was because it helped with the validation of fields and also ensured whatever fields we were fetching matched what the resolvers would typically return.

eseliger · 2023-01-27T18:32:55Z

I didn't mean to say this doesn't have other benefits, it just doesn't give the benefit of the deduplication of security today. I still think this is a valid approach overall to computing these payloads 🤷

BolajiOlajide · 2023-01-27T18:37:08Z

I didn't mean to say this doesn't have other benefits, it just doesn't give the benefit of the deduplication of security today. I still think this is a valid approach overall to computing these payloads 🤷

Yeah, I 💯 agree with that..

courier-new · 2023-01-31T00:01:11Z

Yes, that does feel like a good set of benefits. 🙂

No change of payload if the type changes, making it more stable (yes we have to add it here explicitly, but that actually feels very intentional so I don't dislike that)

I am just a bit concerned about this one, though -- I agree, I like the intentionality behind it, but I worry it'll be very easy to forget to make changes over here when the primary GraphQL type changes, especially if someone other than the 4 of us in this discussion is making the change. It's probably worth us adding some inline comments to the BatchChange and Changeset types to hopefully catch the eye of anyone adding/changing properties there, to at least make them aware of this corresponding payload and remind them to consider making the equivalent changes here.

eseliger · 2023-01-31T19:52:23Z

We could also argue that it gives stability to the payload and that we intentionally expose new fields, and also get alerted (because the query would fail) if a field would be not available anymore which would break the webhook payload API contract. I'm not feeling super strongly about this, but figured I'd leave a note on that here :)

This is a part of #46967. `ForkName` is one of the fields on the webhook payload for a changeset, we don't currently expose `ForkName` via the GraphQL API, this PR fixes that. ## Test plan  * Manually tested * Update unit tests for forked changesets <img width="1199" alt="CleanShot 2023-02-20 at 19 30 43@2x" src="https://user-images.githubusercontent.com/25608335/220178496-7f7a19b6-4c85-4714-8f13-2aceab9cc1dc.png"> --------- Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>

This is also a part of https://github.com/sourcegraph/sourcegraph/pull/46967. We need to expose the id of the batch change that owns a changeset. ## Test plan  * Manually tested * Add unit tests

courier-new

Looks great, thanks Bolaji! Just had 2 minor nits and a field name change suggestion.

courier-new · 2023-02-26T19:03:11Z

+			ID            graphql.ID `json:"id"`
+			Name          string     `json:"name"`
+			Description   string     `json:"description"`
+			State         string     `json:"state"`
+			URL           string     `json:"url"`
+			CreatedAt     time.Time  `json:"createdAt"`
+			UpdatedAt     time.Time  `json:"updatedAt"`
+			LastAppliedAt *time.Time `json:"lastAppliedAt"`
+			ClosedAt      *time.Time `json:"closedAt"`
+			Namespace     struct {
+				ID graphql.ID `json:"id"`
+			} `json:"namespace"`
+			Creator struct {
+				ID graphql.ID `json:"id"`
+			} `json:"creator"`
+			LastApplier struct {
+				ID *graphql.ID `json:"id"`
+			} `json:"lastApplier"`


Nittiest of nits: Would it be possible to order these fields in the same way that they're ordered in the corresponding GraphQL query string? Might make it 10% easier to add additional fields in the future.

Sure thing.

courier-new · 2023-02-26T19:04:48Z

+		externalURL = &node.ExternalURL.URL
+	}
+
+	return json.Marshal(changeset{


Same ordering ask about these fields.

courier-new · 2023-02-26T19:08:35Z

-
-	return json.Marshal(&payload)
+		ForkName:            node.ForkName,
+		OwningBatchChangeID: node.OwnedByBatchChange,


I just noticed, shouldn't these field names match?

Suggested change

OwningBatchChangeID: node.OwnedByBatchChange,

OwnedByBatchChange: node.OwnedByBatchChange,

BolajiOlajide added the batch-changes Issues related to Batch Changes label Jan 26, 2023

BolajiOlajide requested a review from a team January 26, 2023 00:21

BolajiOlajide self-assigned this Jan 26, 2023

cla-bot Bot added the cla-signed label Jan 26, 2023

eseliger approved these changes Jan 26, 2023

View reviewed changes

courier-new reviewed Jan 26, 2023

View reviewed changes

BolajiOlajide mentioned this pull request Feb 20, 2023

batches: expose fork name changesets graphql #47911

Merged

BolajiOlajide mentioned this pull request Feb 20, 2023

batches: expose owning batch change for changesets #47917

Merged

BolajiOlajide added 9 commits February 25, 2023 18:33

add client for graphql queries

744ae3c

work

4fdde1d

handle marshallign changesets

629cd2c

change signature for Enqueue method

f3b63ee

create gql query for changesets

19c7a37

add graphql field to return payload

8bcb38b

expose other changeset fields

7ac7014

pass batch change id

5d1b753

get changeset info from graphq;l

6c52f40

BolajiOlajide added 3 commits February 25, 2023 18:33

abstract request maker

c433674

add test for changeset webhooks

1f4004d

add tests

fa7cd6e

BolajiOlajide force-pushed the bo/construct-webhooks-payload-with-graphql branch from 5accb01 to fa7cd6e Compare February 25, 2023 17:33

BolajiOlajide added 2 commits February 25, 2023 18:36

update fields

763688e

remove ineffective type casting

a3f75b9

courier-new approved these changes Feb 26, 2023

View reviewed changes

BolajiOlajide added 3 commits February 27, 2023 11:25

remove redundant type cast

e2c5d94

change field name:

1d67ca9

reorder fields

9e78379

BolajiOlajide merged commit 906cc90 into main Feb 27, 2023

BolajiOlajide deleted the bo/construct-webhooks-payload-with-graphql branch February 27, 2023 11:20

	OwningBatchChangeID: node.OwnedByBatchChange,
	OwnedByBatchChange: node.OwnedByBatchChange,

Uh oh!

Conversation

BolajiOlajide commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

sourcegraph-bot commented Jan 26, 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!

courier-new commented Jan 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eseliger commented Jan 26, 2023

Uh oh!

Piszmog commented Jan 26, 2023

Uh oh!

eseliger commented Jan 27, 2023

Uh oh!

BolajiOlajide commented Jan 27, 2023

Uh oh!

eseliger commented Jan 27, 2023

Uh oh!

BolajiOlajide commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

courier-new commented Jan 31, 2023

Uh oh!

eseliger commented Jan 31, 2023

Uh oh!

courier-new left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BolajiOlajide commented Jan 26, 2023 •

edited

Loading

sourcegraph-bot commented Jan 26, 2023 •

edited

Loading

BolajiOlajide commented Jan 27, 2023 •

edited

Loading