-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Import GitLab projects #5120
Import GitLab projects #5120
Conversation
import { Gitlab } from "@gitbeaker/node"; | ||
|
||
@injectable() | ||
export class GitLabAppSupport { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The separation in EE and non-EE and the scope of the GitLabService
binding makes it impossible to use that for the additions, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for pushing this forward @AlexTugarev! 🍊
Not sure if this was ready to review but left some early comments below. 🍋
@inject(UserDB) protected readonly userDB: UserDB; | ||
@inject(TokenProvider) protected readonly tokenProvider: TokenProvider; | ||
|
||
async getProviderRepositoriesForUser(params: { user: User, provider: string, hints?: object }): Promise<ProviderRepository[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This is probably minor for this iteration, as this is an edge case, but it takes 5-6 seconds for me to load the next step. FWIW, I'm a member of ~30 groups. In the future a loading indicator could help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for the hint. Just addmin perPage: 100
to override the default value of 20
. That should be significant improvement in your case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AlexTugarev! Seems 2x faster! FWIW, Still taking 2-3 seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. How many projects do you see there? According to the logs, each API response takes ~600ms, which is really slow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most groups contain less than 10 projects. For the personal account all projects (38) are listed. 💯
thought: I think the delay happens while fetching all groups or the ones with proper permissions and maybe because the account is a direct member of the gitlab-org
group which contains 300+ second-level groups including public and private groups. Tried this with a test account and groups and projects loaded in under 1 second. 🎉
Removing the automatic review request because this is still Draft. 😊 |
5b87dab
to
f6990e7
Compare
@inject(UserDB) protected readonly userDB: UserDB; | ||
@inject(TokenProvider) protected readonly tokenProvider: TokenProvider; | ||
|
||
async getProviderRepositoriesForUser(params: { user: User, provider: string, hints?: object }): Promise<ProviderRepository[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AlexTugarev! Seems 2x faster! FWIW, Still taking 2-3 seconds.
/hold |
79d9a8f
to
55b86f3
Compare
Looking at this now! 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AlexTugarev! 🦊
Did another pass and left some comments below. 🏀
@@ -322,7 +371,7 @@ export default function NewProject() { | |||
|
|||
return (<div className="flex flex-col w-96 mt-24 mx-auto items-center"> | |||
<h1>New Project</h1> | |||
<p className="text-gray-500 text-center text-base">Select a git repository.</p> | |||
<p className="text-gray-500 text-center text-base">Select a git repository on <strong>{provider}</strong>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Worth linking to the provider itself? Feel free to ignore if it does not seem helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: Opened follow up issue in #5333
{icon && ( | ||
<img src={icon} className="rounded-full w-6 h-6 absolute top-1/4 left-4" /> | ||
)} | ||
<input className="w-full px-12 cursor-pointer font-semibold" readOnly type="text" value={selectedAccount || ""}></input> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: Opened follow up issue in #5330.
<img src={CaretDown} title="Select Account" className="filter-grayscale absolute top-1/2 right-3" /> | ||
const renderRepos = () => (<> | ||
<div className={`mt-10 border ${isGitHub() ? "rounded-t-xl border-b-0" : "rounded-xl"} border-gray-100 flex-col`}> | ||
<div className="px-8 pt-8 flex flex-col space-y-2"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: For some reason, the pre-selected account is a specific but maybe random group. Does it make sense to default to pre-selecting your own account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaulting to own account sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEAL. Let's do this here or open a follow up issue to fine tune this later! Your call. 📞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: Opened follow up issue in #5331.
38e3465
to
8608b77
Compare
@gtsiolis, thanks for the reminder! first I thought it got so fast, that it's no longer needed ;-) |
Known issue: wrong width for the bottom section. @gtsiolis, wdyt of getting rid of the grayish background and render as simple as in #5120 (comment)? |
Many thanks for adding GitLab support! 💯 🎆 I briefly skimmed the code, but I'm unfamiliar with the GitLab integration code and didn't have time to read more deeply into it. I also briefly tested this PR:
|
1ad647a
to
25b6037
Compare
25b6037
to
485509e
Compare
/werft run 👍 started the job as gitpod-build-at-gitlab-projects.31 |
protected async onDidCreateProject(project: Project) { | ||
let { userId, teamId, cloneUrl } = project; | ||
const parsedUrl = parseRepoUrl(project.cloneUrl); | ||
if ("gitlab.com" === parsedUrl?.host) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work with self-managed gitlab's as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: There's a follow up issue for self hosted support in #5115. Cc @svenefftinge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works well with gitlab.com. From the code besides one place it looks like it should work with self-managed GitLab's too.
LGTM label has been added. Git tree hash: 22ca396613977b4cf0d10117e4d89c57daa1e41a
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: svenefftinge Associated issue: #5278 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@svenefftinge, have you tested prebuilds with self-managed GitLabs? |
/hold cancel |
This PR enables importing of GitLab repositories as projects into Gitpod.
Your GitLab account needs to have at the access level of a maintainer in order to install webhooks for prebuilds. That's also a filter criteria for repositories to be choose from.
Closes #5278