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

batches: Make relation between batch spec and batch change explicit in server-side execution #38921

Merged
BolajiOlajide merged 27 commits into
mainfrom
bo/make-batch-spec-change-relationship-clearer
Jul 26, 2022
Merged

batches: Make relation between batch spec and batch change explicit in server-side execution #38921
BolajiOlajide merged 27 commits into
mainfrom
bo/make-batch-spec-change-relationship-clearer

Conversation

@BolajiOlajide

@BolajiOlajide BolajiOlajide commented Jul 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #36543

Test plan

  • Add unit tests for added functionality.
  • Create batch change server-side and locally and confirm if the column is populated correctly.

Context

This PR adds a new NULLABLE column to batch_specs table that points to batch_changes.id field in the batch_changes table. This column will be populated when the batch spec is created with createBatchSpecFromRaw and when it's not it'll have a value of NULL.

@BolajiOlajide BolajiOlajide self-assigned this Jul 18, 2022
@cla-bot cla-bot Bot added the cla-signed label Jul 18, 2022
@BolajiOlajide BolajiOlajide force-pushed the bo/make-batch-spec-change-relationship-clearer branch from def60dc to 586158e Compare July 19, 2022 03:04
@BolajiOlajide BolajiOlajide marked this pull request as ready for review July 20, 2022 13:41
@BolajiOlajide BolajiOlajide requested a review from a team July 20, 2022 13:41
@sourcegraph-bot

sourcegraph-bot commented Jul 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff bdb58fd...29a7911.

Notify File(s)
@courier-new client/web/src/enterprise/batches/batch-spec/BatchSpecContext.tsx
client/web/src/enterprise/batches/batch-spec/edit/workspaces-preview/useWorkspacesPreview.ts
client/web/src/enterprise/batches/create/ConfigurationForm.tsx
client/web/src/enterprise/batches/create/backend.ts
cmd/frontend/graphqlbackend/batches.go
@efritz enterprise/cmd/worker/internal/batches/workers/reconciler_worker_test.go
@eseliger client/web/src/enterprise/batches/batch-spec/BatchSpecContext.tsx
client/web/src/enterprise/batches/batch-spec/edit/workspaces-preview/useWorkspacesPreview.ts
client/web/src/enterprise/batches/create/ConfigurationForm.tsx
client/web/src/enterprise/batches/create/backend.ts
cmd/frontend/graphqlbackend/batches.go
enterprise/cmd/worker/internal/batches/workers/reconciler_worker_test.go
enterprise/internal/batches/processor/bulk_processor_test.go
enterprise/internal/batches/reconciler/executor_test.go
enterprise/internal/batches/reconciler/reconciler_test.go
enterprise/internal/batches/service/service.go
enterprise/internal/batches/service/service_apply_batch_change_test.go
enterprise/internal/batches/service/service_test.go
enterprise/internal/batches/store/batch_specs.go
enterprise/internal/batches/store/batch_specs_test.go
enterprise/internal/batches/store/changeset_specs_test.go
enterprise/internal/batches/store/changesets_test.go
enterprise/internal/batches/testing/batch_spec.go
enterprise/internal/batches/types/batch_spec.go

Piszmog
Piszmog reviewed Jul 20, 2022
View reviewed changes
Comment thread enterprise/cmd/frontend/internal/batches/resolvers/permissions_test.go
Piszmog
Piszmog approved these changes Jul 20, 2022
View reviewed changes
Comment thread migrations/frontend/1658122170_add_batch_change_id_to_batch_spec/up.sql Outdated
eseliger
eseliger reviewed Jul 25, 2022
View reviewed changes

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great, this does feel much less bad than I thought it might!

I think service.CreateEmptyBatchChange should also set the batch spec id, wdyt? It's a bit annoying to do because there's a circular reference in our DB schema rn with this change, but with the initially deferred constraints, it shouldn't be a big problem.

Comment thread migrations/frontend/1658122170_add_batch_change_id_to_batch_spec/down.sql Outdated
Comment thread migrations/frontend/1658122170_add_batch_change_id_to_batch_spec/up.sql Outdated
Comment thread migrations/frontend/1658122170_add_batch_change_id_to_batch_spec/up.sql Outdated
Comment thread enterprise/internal/batches/service/service.go Outdated
Comment thread cmd/frontend/graphqlbackend/batches.graphql Outdated
Comment thread cmd/frontend/graphqlbackend/batches.graphql Outdated
Comment thread enterprise/cmd/frontend/internal/batches/resolvers/permissions_test.go Outdated
Comment thread enterprise/cmd/frontend/internal/batches/resolvers/resolver_test.go Outdated
Comment thread enterprise/internal/batches/service/service.go Outdated
@BolajiOlajide

BolajiOlajide commented Jul 26, 2022

Copy link
Copy Markdown
Contributor Author

Great, this does feel much less bad than I thought it might!

I think service.CreateEmptyBatchChange should also set the batch spec id, wdyt? It's a bit annoying to do because there's a circular reference in our DB schema rn with this change, but with the initially deferred constraints, it shouldn't be a big problem.

@eseliger service.CreateEmptyBatchChange already sets the batch spec id.

@BolajiOlajide BolajiOlajide force-pushed the bo/make-batch-spec-change-relationship-clearer branch from 6d2d345 to c12cc90 Compare July 26, 2022 03:09
@BolajiOlajide BolajiOlajide requested a review from eseliger July 26, 2022 03:09
@BolajiOlajide BolajiOlajide force-pushed the bo/make-batch-spec-change-relationship-clearer branch from d836dab to 20347b5 Compare July 26, 2022 13:35
LawnGnome
LawnGnome approved these changes Jul 26, 2022
View reviewed changes

@LawnGnome LawnGnome left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1, pending the lint errors being fixed. Nice job!

@eseliger

eseliger commented Jul 26, 2022

Copy link
Copy Markdown
Member
  • sorry, I meant batch change id, on the batch spec. Or am I missing something?

@BolajiOlajide

BolajiOlajide commented Jul 26, 2022

Copy link
Copy Markdown
Contributor Author
  • sorry, I meant batch change id, on the batch spec. Or am I missing something?

@eseliger That'd mean that service.CreateEmptyBatchChange would have a flow similar to:

  • Parse batch spec
  • Create batch spec
  • Create batch change
  • Update created batch spec with batch_change_id

I didn't go that route because the first batch spec for an empty batch change is usually non-executable and just a placeholder until something else gets added. I don't feel strongly about this though but I'm curious as to what you think of the above flow.

@eseliger

eseliger commented Jul 26, 2022

Copy link
Copy Markdown
Member

Yeah that's what I meant with "cyclic" :(
Together with https://github.com/sourcegraph/sourcegraph/issues/38610#issuecomment-1193953442 it might not be necessary, but it would mean we can not take the "new path" in code until then. Not a big deal though.

eseliger
eseliger approved these changes Jul 26, 2022
View reviewed changes
Comment thread enterprise/internal/batches/service/service.go Outdated
@BolajiOlajide

BolajiOlajide commented Jul 26, 2022

Copy link
Copy Markdown
Contributor Author

Yeah that's what I meant with "cyclic" :( Together with #38610 (comment) it might not be necessary, but it would mean we can not take the "new path" in code until then. Not a big deal though.

Yes yes. However, something that could help #38610 is maybe excluding batch specs that have a batch_change_id of NULL. Not sure of the repercussion but that's a potential route one could take.

@BolajiOlajide BolajiOlajide force-pushed the bo/make-batch-spec-change-relationship-clearer branch from ebf4f23 to 29a7911 Compare July 26, 2022 19:32
@BolajiOlajide BolajiOlajide merged commit 01fe70b into main Jul 26, 2022
@BolajiOlajide BolajiOlajide deleted the bo/make-batch-spec-change-relationship-clearer branch July 26, 2022 20:40
@BolajiOlajide BolajiOlajide mentioned this pull request Aug 1, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@LawnGnome LawnGnome LawnGnome approved these changes

@eseliger eseliger eseliger approved these changes

@Piszmog Piszmog Piszmog approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@BolajiOlajide BolajiOlajide

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make relation between batch spec and batch change explicit in server-side execution

5 participants

@BolajiOlajide @sourcegraph-bot @eseliger @LawnGnome @Piszmog