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

batches: use GH App to "duplicate" and sign the commit#52492

Merged
courier-new merged 18 commits into
batches/commit-signingfrom
kr/push-commit-with-app
Jun 5, 2023
Merged

batches: use GH App to "duplicate" and sign the commit#52492
courier-new merged 18 commits into
batches/commit-signingfrom
kr/push-commit-with-app

Conversation

@courier-new

@courier-new courier-new commented May 26, 2023

Copy link
Copy Markdown
Contributor

Closes https://github.com/sourcegraph/sourcegraph/issues/52493.

Here's a walkthrough of the changes. I think it's easier to think about it in this order rather than trying to parse the diff from top to bottom:

  • We add a new method GitHubSource.DuplicateCommit [code].
    • This method is the glue around the client API interactions introduced in https://github.com/sourcegraph/sourcegraph/pull/52749.
    • It looks up the original, unsigned commit produced by gitserver, then constructs a payload for the "create commit" endpoint using metadata from that original commit and issues a POST request to duplicate it. If authenticated with the GH App installation token, this POST request will yield a signed commit.
    • Lastly, it updates the branch ref to point to the new, signed commit.
  • We add another argument on to sourcer.ForChangeset to specify the preferred AuthenticationStrategy [code].
    • Most callers will invoke this method with AuthenticationStrategyUserCredential, which indicates using the "normal" method of authenticating a ChangesetSource with the user or site credential.
    • When the method is invoked with AuthenticationStrategyGitHubApp, if the ChangesetSource would be created for a GitHub code host, we will try to authenticate with a GH App instead (sourcer.withGitHubAppAuthenticator()).
    • This private method will look up if there is any batches GH App for this code host (gitHubAppStore.GetByDomain()).
    • If there is, it will authenticate the ChangesetSource with it as an installation.
    • If there isn't, it'll return an error.
    • Once authenticated in this way, any interactions with the code host through this ChangesetSource (such as creating new commits 👀) will be authenticated as the GH App installation.
  • Finally, we hook into executor.pushChangesetPatch as the entry point to initiate the "duplicate commit workflow" [code].
    • In this part of the code, we already have a PAT-authenticated ChangesetSource for the changeset's repo that we use to create a git push config and push the commit with gitserver.
    • We let gitserver push the commit as normal. We retain the revision ref name (e.g. refs/heads/my-batch-change-branch) to use later to look up the commit, if we will be duplicating it.
    • We call a new private method executor.runAfterCommit with context about the operation.
    • If the ChangesetSource is for a GitHub code host, we try to authenticate a second ChangesetSource for it using a batches GH App.
    • If a batches GH App is configured for this code host, we should be able to authenticate this second source. This clues us in that we should try to sign the commit and proceed with the duplication via GitHubSource.DuplicateCommit().
    • If we weren't able to authenticate because there was no GH App, we break out and return like normal, preserving the original gitserver commit.

Test plan

Updated source tests and added new VCR tests for GitHubSource.DuplicateCommit.

Manually tested the whole workflow with a batches GH App and confirmed a branch with one signed commit was produced at the end:

image

@sourcegraph-buildkite

sourcegraph-buildkite commented May 26, 2023

Copy link
Copy Markdown
Collaborator

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.00% (+0.39 kb) 0.00% (+0.39 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 18b2e87 and 60d130d 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

@BolajiOlajide BolajiOlajide force-pushed the batches/commit-signing branch from e356e40 to b148eb6 Compare May 26, 2023 19:52
@courier-new courier-new force-pushed the kr/push-commit-with-app branch 2 times, most recently from 558f174 to e8f9544 Compare June 1, 2023 01:44
@courier-new courier-new changed the base branch from batches/commit-signing to kr/commit-signing-new-client-methods June 1, 2023 05:25
@courier-new courier-new force-pushed the kr/push-commit-with-app branch from e8f9544 to 7ebcf8d Compare June 1, 2023 05:29
@courier-new courier-new requested a review from a team June 1, 2023 06:44
@courier-new courier-new force-pushed the kr/push-commit-with-app branch 3 times, most recently from 6a49cc0 to a52a3da Compare June 2, 2023 00:53
@courier-new courier-new force-pushed the kr/commit-signing-new-client-methods branch from f11ac01 to c4dc5b4 Compare June 2, 2023 00:53
@courier-new courier-new force-pushed the kr/push-commit-with-app branch 2 times, most recently from 40ef9fb to 83d45a4 Compare June 2, 2023 02:34
@courier-new courier-new force-pushed the kr/commit-signing-new-client-methods branch from c4dc5b4 to 8245682 Compare June 2, 2023 04:00
@courier-new courier-new force-pushed the kr/push-commit-with-app branch from 83d45a4 to 3d2f8c8 Compare June 2, 2023 04:04
@courier-new courier-new marked this pull request as ready for review June 2, 2023 04:23
@sourcegraph-bot

sourcegraph-bot commented Jun 2, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 048be3d...e3b2686.

Notify File(s)
@eseliger enterprise/internal/batches/reconciler/executor.go
enterprise/internal/batches/sources/BUILD.bazel
enterprise/internal/batches/sources/github.go
enterprise/internal/batches/sources/github_test.go
enterprise/internal/batches/sources/mocks_test.go
enterprise/internal/batches/sources/sources.go
enterprise/internal/batches/sources/sources_test.go
enterprise/internal/batches/sources/testdata/sources/GithubSource_DuplicateCommit_invalid_ref.yaml
enterprise/internal/batches/sources/testdata/sources/GithubSource_DuplicateCommit_success.yaml
enterprise/internal/batches/sources/testing/fake.go
enterprise/internal/batches/store/BUILD.bazel
enterprise/internal/batches/store/store.go
enterprise/internal/batches/syncer/BUILD.bazel
enterprise/internal/batches/syncer/mocks_test.go
enterprise/internal/batches/syncer/store.go
enterprise/internal/batches/syncer/syncer.go

@sourcegraph-bot

sourcegraph-bot commented Jun 2, 2023

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

@st0nebreaker st0nebreaker 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.

Looks good! By the way, the "[code]" links in the PR description are broken. I went through the main functions changed and just left a few comments/questions

Comment thread enterprise/internal/batches/sources/github.go Outdated
Comment thread enterprise/internal/batches/reconciler/executor.go Outdated
Comment thread enterprise/internal/batches/sources/github.go Outdated
Comment thread enterprise/internal/batches/reconciler/executor.go Outdated
Base automatically changed from kr/commit-signing-new-client-methods to batches/commit-signing June 2, 2023 20:21
@courier-new

Copy link
Copy Markdown
Contributor Author

Ack, sorry the links were broken! I've hopefully fixed them up now for when Bolaji reviews. 😅

@courier-new courier-new force-pushed the kr/push-commit-with-app branch from 704ded8 to 39d663f Compare June 2, 2023 21:00

@BolajiOlajide BolajiOlajide 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.

This changes look good to me.

@courier-new courier-new merged commit b154242 into batches/commit-signing Jun 5, 2023
@courier-new courier-new deleted the kr/push-commit-with-app branch June 5, 2023 22:43
@st0nebreaker

st0nebreaker commented Jun 6, 2023

Copy link
Copy Markdown
Contributor

Following up for #52224, does this give us any new field on the BatchChange object to indicate the commit was signed I can use for the "Verified" badge @courier-new ? Or if not, is it possible to expose a field like this? 🙏🏻

@courier-new

courier-new commented Jun 6, 2023

Copy link
Copy Markdown
Contributor Author

No, nothing for that yet! But to give you a starting place, the extsvc/github.restCommit type, which is the response object we get back from GitHub when we hit their "create commit" endpoint, has a Verification property which should contain information about whether or not the commit was signed. If you add a column to the changeset_specs table like commit_verification and make it jsonb type, we could record that verification when we're doing the duplicate commit thing and eventually surface it to the UI!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

batch-changes Issues related to Batch Changes cla-signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants