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

[Backport 5.2] batches: fix commit signing with changeset forks#57597

Merged
keegancsmith merged 1 commit into
5.2from
backport-57520-to-5.2
Oct 14, 2023
Merged

[Backport 5.2] batches: fix commit signing with changeset forks#57597
keegancsmith merged 1 commit into
5.2from
backport-57520-to-5.2

Conversation

@sourcegraph-release-bot

Copy link
Copy Markdown
Collaborator

Context

The customer reported an issue where a Sourcegraph instance configured with a GitHub app for Batch Changes couldn't sign commits on forked changesets even when the GitHub app was granted access to the forked repo.

This happens because while creating an authenticator used to create the signed commits, we always assume the repository to find the GitHub app installation for is the original repository where the Batch CHange was executed.

https://github.com/sourcegraph/sourcegraph/blob/main/internal/batches/sources/sources.go#L161

In this PR, I changed the approach and instead checked if the changeset is pushed to a fork, then figured out the namespace where the forked changeset is created and used that to get the GitHub app installation record.

CleanShot 2023-10-11 at 11 49 50@2x

Test plan

  • MAnual testing
  • Add a GitHub app for BAtch Changes commit signing. Ensure you give it access to the forked repository of your choice.
  • Add batchChanges.enforceForks to your site config, and create a batch change.
  • The changeset should be verified and created in your repository fork once published.

[Context](https://sourcegraph.slack.com/archives/C04UV9ZVBB2/p1696374182439959)

The customer reported an issue where a Sourcegraph instance configured with a GitHub app for Batch Changes couldn't sign commits on forked changesets even when the GitHub app was granted access to the forked repo.

This happens because while creating an authenticator used to create the signed commits, we always assume the repository to find the GitHub app installation for is the original repository where the Batch CHange was executed.

https://github.com/sourcegraph/sourcegraph/blob/main/internal/batches/sources/sources.go#L161

In this PR, I changed the approach and instead checked if the changeset is pushed to a fork, then figured out the namespace where the forked changeset is created and used that to get the GitHub app installation record.

![CleanShot 2023-10-11 at 11 49 50@2x](https://github.com/sourcegraph/sourcegraph/assets/25608335/3f90937e-6d1f-4631-8ce4-ca7a06a8423f)

## Test plan

* MAnual testing

- Add a GitHub app for BAtch Changes commit signing. Ensure you give it access to the forked repository of your choice.
- Add `batchChanges.enforceForks` to your site config, and create a batch change.
- The changeset should be verified and created in your repository fork once published.

* Updated unit tests

(cherry picked from commit 8eb5d83)
@sourcegraph-bot

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 461cf0e...2555c92.

Notify File(s)
@eseliger internal/batches/reconciler/executor.go
internal/batches/sources/mocks_test.go
internal/batches/sources/sources.go
internal/batches/sources/sources_test.go
internal/batches/sources/testing/fake.go
internal/batches/syncer/mocks_test.go
internal/batches/syncer/store.go
internal/batches/syncer/syncer.go

@sourcegraph-bot

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

@keegancsmith keegancsmith added the backport/bugfix Standard patches to fix bugs label Oct 14, 2023
@keegancsmith

Copy link
Copy Markdown
Member

Changelog for 5.2.1 on main pls

@keegancsmith keegancsmith merged commit 5775b5e into 5.2 Oct 14, 2023
@keegancsmith keegancsmith deleted the backport-57520-to-5.2 branch October 14, 2023 05:43
@BolajiOlajide

Copy link
Copy Markdown
Contributor

Changelog for 5.2.1 on main pls

Waiting for a 👍🏾 here: https://github.com/sourcegraph/sourcegraph/pull/57598. It'll be in main soon.
Thanks

@varungandhi-src varungandhi-src mentioned this pull request Jan 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants