Skip to content

Adding getSuggestedRepositories method #18681

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

Merged
merged 10 commits into from
Sep 12, 2023

Conversation

selfcontained
Copy link
Contributor

@selfcontained selfcontained commented Sep 7, 2023

Description

This is part of work to include projects on the create workspace page. Functionality is gated behind the includeProjectsOnCreateWorkspace feature flag and is only on in non-prod. When it is on, it will data from the new api, and incorporate it into the existing UI. Otherwise it will continue to use getSuggestedContextUrls

Adding a new getSuggestedRepositories method to server that returns similar items to getSuggestedContextUrls, except in the form of structured data to include project details (if there is a project).

export type SuggestedRepository = {
    url: string;
    projectId?: string;
    projectName?: string;
};

Followup work will update the UI based on the new designs to incorporate projects information.

Summary generated by Copilot

🤖 Generated by Copilot at a124119

This pull request adds a new feature to suggest repositories for the user based on their organization and projects when creating a workspace. It also adds a new feature flag to toggle between the server-based and the local storage-based suggestions. It modifies the RepositoryFinder component, the GitpodServer interface, and the GitpodService class to implement this feature. It also adds new types, hooks, and queries to handle the suggested repositories data.

Related Issue(s)

Fixes EXP-583

How to test

  • Visit the Create Workspace page
  • Everything should look like normal, even though it's using the new getSuggestedRepositories api

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

Build Options

Build
  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer
  • analytics=segment
  • with-dedicated-emulation
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
  • with-monitoring

/hold

@@ -1736,6 +1737,146 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
.map((s) => s.url);
}

public async getSuggestedRepositories(ctx: TraceContext, organizationId: string): Promise<SuggestedRepository[]> {
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this method into a new service SCMService and call it from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we have an ScmService so I'll go ahead and add it there. Let me know if that doesn't seem like the right place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you suggest for the few methods from gitpod-server-impl that this currently depends on?

  • getAuthProviders() - Not sure on this one, should it all move to the AuthProviderService behind a method there?
  • getWorkspaces() - mostly just a call to workspaceService.getWorkspaces() except for the guardAccess checks against each workspace.

Copy link
Member

Choose a reason for hiding this comment

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

The getAuthProivider should probably move to AuthProviderService. The SCMService could use the workspaceService to fetch the workspaces.
But if this is too much refactoring, I'm also fine with keeping this in gitpod-server-impl for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went down the route of pulling it into the SCMService and getAuthProviders into AuthProviderService, but then bumped into cyclic deps issues between SCMSerivce/AuthProviderService and ProjectService, so backed up and will consider that a future improvement.

@@ -1736,6 +1737,146 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
.map((s) => s.url);
}

public async getSuggestedRepositories(ctx: TraceContext, organizationId: string): Promise<SuggestedRepository[]> {
const user = await this.checkAndBlockUser("getSuggestedRepositories");
await this.guardWithFeatureFlag("includeProjectsOnCreateWorkspace", user, organizationId);
Copy link
Member

Choose a reason for hiding this comment

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

why do we guard by feature flag here? wouldn't it be better to guard on the UI side? There doesn't seem a ton of value in throwing errors for some users when they are not enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The UI is guarded too, I can remove it here.

lastUse?: string;
};

const fetchExampleRepos = async (): Promise<SuggestedRepositoryWithSorting[]> => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add the example repos anymore. I don't think they are helpful if we can show stuff that belongs to the user. cc @loujaybee

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for highlighting, Sven! I'm not fully sure what removing the demo repos would mean, or the consequence? Where currently in the product do we surface the example repos today? What would be the impact/change?

if we can show stuff that belongs to the user.

If I understand correctly, I agree. I'd say for most of our customers we should work on the assumption that there will be a central team that wants to actively manage and curate the experience, and that could include making projects that are examples or demos for their teams. That could be something that we could bootstrap our organisations with (a couple example real projects) as part of customer onboarding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@loujaybee In this case it would exclude example repos in what shows up in the repo selector on the create workspace page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Re-posting two of the current problems with the example repositories:

  1. Users who haven't authorized GitHub.com, see none of them in /new, including users using GitHub Enterprise, see Templates list is empty for users that only authorized with a GitLab #3531 and relevant discussion (internal). ❌
  2. From those users who can see them in /new, they don't have permissions to push (write access) for these repositories, making them a bit unusable as they have to fork or add a git remote before pushing any changes. ❌

@svenefftinge
Copy link
Member

The suggestedContextURL query is very slow. I think we should identify which of the queries (I suspect the SCM one) is making this slow and see if we can optimize or cache.

@roboquat roboquat added size/XL and removed size/L labels Sep 11, 2023
@selfcontained selfcontained force-pushed the brad/exp-583-add-new-getsuggestedprojects-api branch from bbb63c9 to 5c84263 Compare September 11, 2023 17:47
@roboquat roboquat added size/L and removed size/XL labels Sep 11, 2023
@selfcontained selfcontained marked this pull request as ready for review September 12, 2023 15:09
@selfcontained selfcontained requested a review from a team as a code owner September 12, 2023 15:09
@selfcontained
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit fa3010a into main Sep 12, 2023
@roboquat roboquat deleted the brad/exp-583-add-new-getsuggestedprojects-api branch September 12, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants