Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[db/server] Link workspaces and projects #4846

Merged
merged 2 commits into from
Jul 20, 2021
Merged

[db/server] Link workspaces and projects #4846

merged 2 commits into from
Jul 20, 2021

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Jul 16, 2021 •
edited
Loading

We're now linking projects to workspaces as well.

Also in this PR:

  • adding index on d_b_project.cloneUrl
  • removing Without type (replaceable with Omit)
  • refactored ProjectDB.storeProject and Project.create

@AlexTugarev AlexTugarev requested a review from jankeromnes July 16, 2021 08:46
@AlexTugarev AlexTugarev requested a review from svenefftinge July 16, 2021 08:46
@AlexTugarev
Copy link
Member Author

AlexTugarev commented Jul 16, 2021 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-at-workspace-project.2

@AlexTugarev AlexTugarev force-pushed the at/workspace-project branch from 382a2d9 to 3ae977b Compare July 16, 2021 08:59
@jankeromnes jankeromnes mentioned this pull request Jul 16, 2021
Merged
geropl
geropl reviewed Jul 19, 2021
View reviewed changes
components/gitpod-db/src/typeorm/project-db-impl.ts Outdated
public async findProjectsByTeam(teamId: string): Promise<Project[]> {
const repo = await this.getRepo();
return repo.find({ teamId });
}

public async createProject(name: string, cloneUrl: string, teamId: string, appInstallationId: string): Promise<Project> {
const repo = await this.getRepo();
if (repo.findOne({ cloneUrl })) {
throw new Error('A project with the same clone URL already exists');
}

const project: Project = {
id: uuidv4(),
Copy link
Member

Choose a reason for hiding this comment

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

Personally I like to decouple such creation logic from the DB, where it is hard to test etc. One example is to move this into a method Project.new in project-protocol.ts which takes a Without<Project, 'id' | 'creationTime'>.

Copy link
Member Author

Choose a reason for hiding this comment

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

you're talking about externalizing the creation of Project object to be stored afterwards here.
then we can if id included and add the check.
that sounds good.

@geropl
Copy link
Member

geropl commented Jul 19, 2021 •
edited
Loading

The first thing strikes me a bit odd: A workspaces I always 1:0..1 to a project? 🤔 Without thinking deeply about it I always thought about it as 1:0...n - but would need to think about concrete usecases first.

@jankeromnes Left some remarks. I'm not sure I'm qualified to 'lgtm' before I did some more reading on "Teams & Projects" . 🙂

Update: See #4846 (comment)

@AlexTugarev AlexTugarev force-pushed the at/workspace-project branch from 3ae977b to 25213a2 Compare July 20, 2021 08:32
@roboquat roboquat added size/XL and removed size/M labels Jul 20, 2021
@AlexTugarev
Copy link
Member Author

@jankeromnes, please have another pass on this.

jankeromnes
jankeromnes approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Looks good to me 😊 thanks!

components/gitpod-protocol/src/teams-projects-protocol.ts
export interface PrebuildInfo {
id: string;
teamId: string;
project: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

projectId would be more correct, but this is unimportant

@roboquat roboquat added the lgtm label Jul 20, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 10962e03610ebe1d13d7b24d55c3e53ffb3711e6

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexTugarev, jankeromnes

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 313fc79 into main Jul 20, 2021
@roboquat roboquat deleted the at/workspace-project branch July 20, 2021 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geropl geropl geropl left review comments

@jankeromnes jankeromnes jankeromnes approved these changes

@svenefftinge svenefftinge Awaiting requested review from svenefftinge

Assignees

@jankeromnes jankeromnes

Labels
approved roadmap item: teams & projects size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AlexTugarev @geropl @roboquat @svenefftinge @jankeromnes