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

batches: fix commit signing with changeset forks#57520

Merged
BolajiOlajide merged 10 commits into
mainfrom
bo/fix-commit-signing-with-forks
Oct 12, 2023
Merged

batches: fix commit signing with changeset forks#57520
BolajiOlajide merged 10 commits into
mainfrom
bo/fix-commit-signing-with-forks

Conversation

@BolajiOlajide

@BolajiOlajide BolajiOlajide commented Oct 10, 2023

Copy link
Copy Markdown
Contributor

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.
  • Updated unit tests

@cla-bot cla-bot Bot added the cla-signed label Oct 10, 2023
@BolajiOlajide BolajiOlajide force-pushed the bo/fix-commit-signing-with-forks branch from 360027d to e9dba11 Compare October 11, 2023 13:47
@BolajiOlajide BolajiOlajide self-assigned this Oct 11, 2023
@BolajiOlajide BolajiOlajide requested a review from a team October 11, 2023 16:25
@BolajiOlajide BolajiOlajide marked this pull request as ready for review October 11, 2023 16:25
@sourcegraph-bot

sourcegraph-bot commented Oct 11, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 6c70bc1...8fc845b.

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

Comment thread internal/github_apps/store/store.go
Comment thread internal/batches/sources/sources.go
Comment thread internal/batches/sources/sources.go Outdated
BolajiOlajide and others added 2 commits October 11, 2023 16:09
Co-authored-by: Camden Cheek <camden@ccheek.com>
@BolajiOlajide BolajiOlajide merged commit 8eb5d83 into main Oct 12, 2023
@BolajiOlajide BolajiOlajide deleted the bo/fix-commit-signing-with-forks branch October 12, 2023 10:58
BolajiOlajide added a commit that referenced this pull request Oct 12, 2023
sourcegraph-release-bot pushed a commit that referenced this pull request Oct 13, 2023
[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)
keegancsmith pushed a commit that referenced this pull request Oct 14, 2023
batches: fix commit signing with changeset forks (#57520)

[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)

Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com>
@peterguy peterguy added the batch-changes Issues related to Batch Changes label Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport 5.2 batch-changes Issues related to Batch Changes cla-signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants