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

Include GitLab subgroups #6546

Merged
merged 1 commit into from
Nov 5, 2021
Merged

Include GitLab subgroups #6546

merged 1 commit into from
Nov 5, 2021

Conversation

laushinka
Copy link
Contributor

@laushinka laushinka commented Nov 3, 2021 •
edited
Loading

Fixes #6068

Description

This PR allows GitLab projects in subgroups and nested subgroups to be accessible. This PR also includes additional fixes related to the GitLab slug issue.

Related Issue(s)

Fixes #6068

How to test

  1. Create a subgroup and a subgroup inside that subgroup (however nested you like) in GitLab
  2. Create a project within each of those subgroups
  3. Add those projects in Gitpod. Run prebuilds for each of them.

Release Notes

Prebuilds can run for GitLab subgroup projects.

Documentation

@svenefftinge
Copy link
Member

Multiple subgroups can be nested (docs).
Would be great to add unit tests for the changed function.

@laushinka
Copy link
Contributor Author

laushinka commented Nov 4, 2021 •
edited
Loading

Multiple subgroups can be nested (docs). Would be great to add unit tests for the changed function.

@svenefftinge Thanks for this!

@laushinka
Copy link
Contributor Author

/hold

@laushinka laushinka removed the request for review from geropl November 4, 2021 11:15
@roboquat roboquat added size/L and removed size/XS labels Nov 4, 2021
@laushinka
Copy link
Contributor Author

/unhold

Ready for another review.

gtsiolis
gtsiolis approved these changes Nov 4, 2021
View reviewed changes
Copy link
Contributor

@gtsiolis gtsiolis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

UX works as expected, @laushinka! 🔮

Approving to unblock merging but holding 🅰️ because of a minor non-blocking issue and 🅱️ in case we need someone to take a closer look at the code.

/hold

components/server/src/repohost/repo-url.spec.ts
});
}

@test public parseSubgroupFourLevels() {
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Thanks for adding these tests here, @laushinka! 🌟

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to @svenefftinge for the reminder to write more tests :)

components/server/src/repohost/repo-url.spec.ts

@test public parseSubgroupFourLevels() {
const testUrl = RepoURL.parseRepoUrl(
"https://gitlab.com/hello-group/my-subgroup/my-sub-subgroup/my-sub-sub-subgroup/my-sub-sub-sub-subgroup/my-cool-project.git")
Copy link
Contributor

Choose a reason for hiding this comment

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

issue(non-blocking): Cross-posting from #5362 (comment) to bring this up again, although sounds ok to leave this out of the scope of this PR, in case we'd like to make plan any action:

Shall we move this group under gitpod-io group?

Cc @corneliusludmann @geropl

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'd love to answer but I'm not sure I know what this question or this context means, even after reading the issue. Maybe something I can ask about in the next team call? Also cc-ing @JanKoehnlein in case he has context.

@roboquat roboquat added the lgtm label Nov 4, 2021
@roboquat
Copy link
Contributor

roboquat commented Nov 4, 2021

LGTM label has been added.

Git tree hash: 9e8c123b4e2aaad7aaa0a6aad16178f93df67796

@JanKoehnlein
Copy link
Contributor

Doesn't work for me. I created group0 and group1 inside and projects group0/bar and group0/group1/Foo. The latter can be prebuilt, the former can't. Could be related to the fact that GitLab changed the URL of group0 to g2506 to avoid collision.

@laushinka
Copy link
Contributor Author

laushinka commented Nov 4, 2021 •
edited by werft-gitpod-dev-com bot
Loading

Could be related to the fact that GitLab changed the URL of group0 to g2506 to avoid collision.

😵 @JanKoehnlein show in a call?

@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 4, 2021 •
edited
Loading

question: @JanKoehnlein did use the same example project + commit for the two projects? Cc @laushinka

issue(non-blocking): I could not prebuild the second project when using the same commit, but this is a different issue related to prebuilds and out of the scope of this PR, right?

Got the following error:

Could not run prebuild.
A prebuild is already running for this commit.

@JanKoehnlein
Copy link
Contributor

We just found out that the issues are likely connected to the fact that my project was called Foo and compared against the slug foo was not matched.

@roboquat roboquat removed the lgtm label Nov 4, 2021
@laushinka
Copy link
Contributor Author

laushinka commented Nov 4, 2021 •
edited by werft-gitpod-dev-com bot
Loading

We just found out that the issues are likely connected to the fact that my project was called Foo and compared against the slug foo was not matched.

@JanKoehnlein Just pushed. Please help test again with your test cases 🙏🏽

@roboquat roboquat added team: workspace Issue belongs to the Workspace team and removed approved labels Nov 5, 2021
@laushinka laushinka removed the team: workspace Issue belongs to the Workspace team label Nov 5, 2021
@JanKoehnlein
Copy link
Contributor

/lgtm

Should we also remove ProjectDB.findProjectByTeamAndName()? In its implementation we also filter projects by name ignoring the slug, and it doesn't seem to be called anywhere.

@roboquat roboquat added the lgtm label Nov 5, 2021
@roboquat
Copy link
Contributor

roboquat commented Nov 5, 2021

LGTM label has been added.

Git tree hash: 204efbd1b021ee8df403901e61793a4449d714f6

@laushinka
Copy link
Contributor Author

laushinka commented Nov 5, 2021 •
edited
Loading

/lgtm

@JanKoehnlein Did the prebuilds work now for your projects? I had issues still with a few of them, but not sure if it's because of the specific repos/configs as Chris mentioned in this conversation.

Should we also remove ProjectDB.findProjectByTeamAndName()? In its implementation we also filter projects by name ignoring the slug, and it doesn't seem to be called anywhere.

Good point.

@roboquat roboquat removed the lgtm label Nov 5, 2021
@gitpod-io gitpod-io deleted a comment from codecov bot Nov 5, 2021
@JanKoehnlein
Copy link
Contributor

/lgtm

@roboquat roboquat added the lgtm label Nov 5, 2021
@roboquat
Copy link
Contributor

roboquat commented Nov 5, 2021

LGTM label has been added.

Git tree hash: 9e4daa0e7bb0ba5d55de6466f05fab50e41b1a05

@roboquat
Copy link
Contributor

roboquat commented Nov 5, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gtsiolis, JanKoehnlein

Associated issue: #6068

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

@laushinka
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit de09b5f into main Nov 5, 2021
@roboquat roboquat deleted the laushinka/branch-6068 branch November 5, 2021 12:08
@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 5, 2021

5t2ktu

@jldec jldec added the deployed Change is completely running in production label Nov 14, 2021
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtsiolis gtsiolis gtsiolis approved these changes

@JanKoehnlein JanKoehnlein Awaiting requested review from JanKoehnlein

Assignees

@gtsiolis gtsiolis

@JanKoehnlein JanKoehnlein

Labels
approved deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot fetch branches, trigger prebuilds, and configure projects in a subgroup for GitLab
6 participants
@laushinka @svenefftinge @roboquat @JanKoehnlein @gtsiolis @jldec