[Self-Hosted] Allow integrating with 'github.com' without a GitHub App #9231

jankeromnes · 2022-04-11T16:22:06Z

Description

Allow integrating with 'github.com' without requiring a GitHub App. This simply reuses the same common code as GitLab, Bitbucket and GitHub Enterprise, i.e. API and webhooks.

Related Issue(s)

Fixes #9163

How to test

Don't follow any of the steps from https://www.notion.so/gitpod/How-to-deploy-a-PR-with-a-working-GitHub-App-integration-d297a1ef2f7b4b3aa8483b2ae9b47da2
You should still be able to add Projects and run Prebuilds for github.com repos 🎉

Release Notes

Allow integrating with 'github.com' without a GitHub App

Documentation

jankeromnes · 2022-04-11T18:19:54Z

Left TODO:

Ensure "repo" scope for github.com in New Project flow (when there is no GitHub App)
Fix "no github.com user repos listed" (when there is no GitHub App)

EDIT: Second problem is, listing user repositories here:

gitpod/components/server/ee/src/prebuilds/github-service.ts

Line 29 in 5166afd

    
           const repositories = (await this.githubApi.run(user, (gh) => gh.repos.listForAuthenticatedUser({}))).data;

is paginated -- we only get the first 30.

We could use per_page: 100, and/or paginate by using octokit.paginate (a little bit tricky to integrate with githubApi.run, because paginate wants to work on and return response.data, while githubApi.run wants to be given a response ... need to figure something out)

EDIT: Fixed ✅

jankeromnes · 2022-04-14T08:58:22Z

Alrighty, this is now ready for review! 🎉 Please take a look. 🙏

(Also, please disregard the merge conflict -- I had to base my PR on an older commit because preview envs got broken in main, but I'll rebase again before merging.)

geropl · 2022-04-14T09:06:14Z

Thank you @jankeromnes ! 🚀 Will have a look later today 👀

jankeromnes · 2022-04-19T10:41:54Z

Friendly ping @geropl 😇 🙏

geropl · 2022-04-19T10:58:28Z

Friendly ping @geropl

Thx @jankeromnes ! still have it on the radar, but might get delayed until tomorrow.

geropl · 2022-04-21T11:37:15Z

@jankeromnes Starting now.

geropl · 2022-04-21T11:55:34Z

components/server/ee/src/prebuilds/github-service.ts

@@ -26,7 +26,8 @@ export class GitHubService extends RepositoryService {
    @inject(GithubContextParser) protected githubContextParser: GithubContextParser;

    async getRepositoriesForAutomatedPrebuilds(user: User): Promise<ProviderRepository[]> {
-        const repositories = (await this.githubApi.run(user, (gh) => gh.repos.listForAuthenticatedUser({}))).data;
+        const octokit = await this.githubApi.create(user);
+        const repositories = await octokit.paginate(octokit.repos.listForAuthenticatedUser, { per_page: 100 });


Cool to see there is a library method for this! 👍

We could try to push our map function into paginate to be more conscious about memory usage: https://github.com/octokit/plugin-paginate-rest.js/#octokitpaginate.
Because we do not have a filterMap, it could return ProviderRepository | undefined, followed by a filter(repo => !!) afterwards.

The reason I'm raising this seemingly minor issue is that when looking at COU profiles, our code is riddled with uncertainties about cardinality, leading to performance drops/spikes.
Also happy to review a follow up for this.

Good point! I believe we can simply move our after-the-fact filter into the octokit.paginate call (not sure what you mean with a filterMap, but I think a simple filter might do the trick).

I believe we can simply move our after-the-fact filter into the octokit.paginate

Ah, didn't look close enough to recognize it receives a response, not an individual shape only - nice.

Next step - and the intent of my message - was to also move our map function into paginate, so we can drop all fields we do not need before accumulating them in the repositories array. But again, this is more of a nit. Let's see to merge this as-is to not have us blocked longer than necessary by re-build issues 🙄

Ah, whoops, the map would indeed have a much stronger memory reduction effect than the filter alone. 💡 Follow-up issue to the rescue!

EDIT: #9461

geropl

Approving now despite the conflic because my test worked flawlessly, and I like the code as is!

Please ping once the conflict is resolved.

…eating a New Project

… Project flow

jankeromnes · 2022-04-21T13:22:42Z

Rebased (without change) and moved the filter into octokit.paginate.

Will request re-approval if the deployment still works.

geropl · 2022-04-21T13:38:46Z

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-jx-no-app.16

I noticed this error in the previous run:

[Validating configuration]     {
[Validating configuration]       "name": "proxy-config-certificates is present and valid",
[Validating configuration]       "description": "ensures the proxy-config-certificates secret is present and contains the required data",
[Validating configuration]       "status": "ERROR",
[Validating configuration]       "errors": [
[Validating configuration]         {
[Validating configuration]           "message": "secret proxy-config-certificates not found",
[Validating configuration]           "type": "ERROR"
[Validating configuration]         }
[Validating configuration]       ]
[Validating configuration]     },

with-clean-slate-deployment=true helped me with that before, so: 🤞

geropl

#9231 (review)

roboquat added release-note do-not-merge/work-in-progress size/S labels Apr 11, 2022

jankeromnes force-pushed the jx/no-app branch from 8390718 to 6b5cffe Compare April 11, 2022 16:25

jankeromnes force-pushed the jx/no-app branch from 6b5cffe to 7473919 Compare April 12, 2022 08:56

roboquat added size/M and removed size/S labels Apr 12, 2022

jankeromnes force-pushed the jx/no-app branch from 7473919 to ac66ff9 Compare April 12, 2022 09:36

roboquat added size/L and removed size/M labels Apr 12, 2022

jankeromnes force-pushed the jx/no-app branch 4 times, most recently from a63a602 to 392eff3 Compare April 13, 2022 14:46

roboquat added size/XL and removed size/L labels Apr 13, 2022

jankeromnes force-pushed the jx/no-app branch 2 times, most recently from dc4448e to b255b97 Compare April 14, 2022 08:16

roboquat added size/L and removed size/XL labels Apr 14, 2022

jankeromnes force-pushed the jx/no-app branch 2 times, most recently from 44212e2 to 0632c15 Compare April 14, 2022 08:57

jankeromnes marked this pull request as ready for review April 14, 2022 08:58

jankeromnes requested a review from a team April 14, 2022 08:58

roboquat removed the do-not-merge/work-in-progress label Apr 14, 2022

jankeromnes assigned geropl Apr 15, 2022

geropl added the team: webapp Issue belongs to the WebApp team label Apr 20, 2022

geropl reviewed Apr 21, 2022

View reviewed changes

geropl previously approved these changes Apr 21, 2022

View reviewed changes

jankeromnes added 2 commits April 21, 2022 13:14

Allow integrating with 'github.com' without a GitHub App

de520b9

[dashboard] Ensure 'github.com' always has the correct scopes when cr…

66b9d14

…eating a New Project

jankeromnes dismissed geropl’s stale review via 2a38bb6 April 21, 2022 13:14

jankeromnes force-pushed the jx/no-app branch from 0632c15 to 2a38bb6 Compare April 21, 2022 13:14

[server] Paginate GitHub API request listing user repositories in New…

06e6157

… Project flow

jankeromnes force-pushed the jx/no-app branch from 2a38bb6 to 06e6157 Compare April 21, 2022 13:20

geropl approved these changes Apr 21, 2022

View reviewed changes

roboquat merged commit d1bb3bd into main Apr 21, 2022

roboquat deleted the jx/no-app branch April 21, 2022 13:56

This was referenced Apr 21, 2022

Optimization: Map GitHub API repositories during pagination, not after #9461

Closed

[server] Optimization: Map GitHub API repositories during pagination, not after #9462

Merged

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Apr 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Self-Hosted] Allow integrating with 'github.com' without a GitHub App #9231

[Self-Hosted] Allow integrating with 'github.com' without a GitHub App #9231

jankeromnes commented Apr 11, 2022

jankeromnes commented Apr 11, 2022 •

edited

Loading

jankeromnes commented Apr 14, 2022 •

edited

Loading

geropl commented Apr 14, 2022

jankeromnes commented Apr 19, 2022 •

edited by werft-gitpod-dev-com bot

Loading

geropl commented Apr 19, 2022

geropl commented Apr 21, 2022

geropl Apr 21, 2022

jankeromnes Apr 21, 2022

geropl Apr 21, 2022

jankeromnes Apr 21, 2022 •

edited

Loading

geropl left a comment

jankeromnes commented Apr 21, 2022

geropl commented Apr 21, 2022 •

edited

Loading

geropl left a comment

[Self-Hosted] Allow integrating with 'github.com' without a GitHub App #9231

[Self-Hosted] Allow integrating with 'github.com' without a GitHub App #9231

Conversation

jankeromnes commented Apr 11, 2022

Description

Related Issue(s)

How to test

Release Notes

Documentation

jankeromnes commented Apr 11, 2022 • edited Loading

jankeromnes commented Apr 14, 2022 • edited Loading

geropl commented Apr 14, 2022

jankeromnes commented Apr 19, 2022 • edited by werft-gitpod-dev-com bot Loading

geropl commented Apr 19, 2022

geropl commented Apr 21, 2022

geropl Apr 21, 2022

Choose a reason for hiding this comment

jankeromnes Apr 21, 2022

Choose a reason for hiding this comment

geropl Apr 21, 2022

Choose a reason for hiding this comment

jankeromnes Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

geropl left a comment

Choose a reason for hiding this comment

jankeromnes commented Apr 21, 2022

geropl commented Apr 21, 2022 • edited Loading

geropl left a comment

Choose a reason for hiding this comment

jankeromnes commented Apr 11, 2022 •

edited

Loading

jankeromnes commented Apr 14, 2022 •

edited

Loading

jankeromnes commented Apr 19, 2022 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes Apr 21, 2022 •

edited

Loading

geropl commented Apr 21, 2022 •

edited

Loading