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

batches: Specify domain when creating a Github App#52347

Merged
st0nebreaker merged 1 commit into
batches/commit-signingfrom
bo/create-batches-github-app
May 25, 2023
Merged

batches: Specify domain when creating a Github App#52347
st0nebreaker merged 1 commit into
batches/commit-signingfrom
bo/create-batches-github-app

Conversation

@BolajiOlajide

Copy link
Copy Markdown
Contributor

Closes #52346

This PR allows the domain to be specified when creating a GitHub app. Right now it's hard coded to only work with the repos domain till we figure out the UX for creating a Batch Changes GitHub App.

Test plan

  • Create a GitHub app.
  • inspect the github_apps table for the newly created GitHub app - the app should have the value of repos in the domain column.

@BolajiOlajide BolajiOlajide requested review from a team and kopancek May 24, 2023 04:07
@BolajiOlajide BolajiOlajide self-assigned this May 24, 2023
@cla-bot cla-bot Bot added the cla-signed label May 24, 2023
@BolajiOlajide BolajiOlajide changed the title batches: Add domain column to github_apps migration (#52332) batches: Specify domain when creating a Github App May 24, 2023
@sourcegraph-bot

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 14b81ad...d30ab10.

Notify File(s)
@unknwon enterprise/cmd/frontend/internal/auth/githubappauth/middleware.go

@sourcegraph-buildkite

Copy link
Copy Markdown
Collaborator

Bundle size report 📦

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

Look at the Statoscope report for a full comparison between the commits d30ab10 and 40870a1 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

@sourcegraph-bot

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

@courier-new courier-new 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.

👌

WebhookUUID: webhookUUID,
Domain: domain,
}
stateDeets, err := json.Marshal(stateDetails)

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.

Lol I like the name. 😂

@st0nebreaker st0nebreaker self-requested a review May 25, 2023 17:08
return
}

cache.Set(s, stateDeets)

@st0nebreaker st0nebreaker May 25, 2023

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.

Can you explain why prior to this change we had to make webhookUUID a byte slice here, but now with the added domain property we don't have to? @BolajiOlajide

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CleanShot 2023-05-25 at 21 25 24@2x

The set method expects a byte slice as the second argument. The webhookUUID variable is a string, so we needed to typecast it into a byte slice; now that we are saving multiple values (via the struct) in the cache, we call the json.Marshal method, which returns a byte slice so we don't need to type cast. @st0nebraker

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.

Got it, thanks for explaining 😁

@st0nebreaker st0nebreaker merged commit 98bc90f into batches/commit-signing May 25, 2023
@st0nebreaker st0nebreaker deleted the bo/create-batches-github-app branch May 25, 2023 20:25
BolajiOlajide added a commit that referenced this pull request May 26, 2023
Closes [#52346](https://github.com/sourcegraph/sourcegraph/issues/52346)

This PR allows the domain to be specified when creating a GitHub app.
Right now it's hard coded to only work with the `repos` domain till we
figure out the UX for creating a Batch Changes GitHub App.

## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->
* Create a GitHub app.
* inspect the `github_apps` table for the newly created GitHub app - the
app should have the value of `repos` in the `domain` column.
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.

6 participants