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.

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

Introduce project slug #6376

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Introduce project slug #6376

merged 1 commit into from
Nov 2, 2021

Conversation

laushinka
Copy link
Contributor

@laushinka laushinka commented Oct 24, 2021

Description

This PR:

  1. Introduces a slug column for d_b_project
  2. Adds the 'path' attribute provided by the GitLab API on project creation
  3. For existing Github projects, the slug column gets populated according to the name column (because already slugified)

Screenshot 2021-10-25 at 01 22 18

Related Issue(s)

Maybe partially fixes #5847 because this PR does not tackle existing GitLab projects. Detailed thoughts in the issue here.

How to test

  1. Create a GitLab project in a GitLab account
  2. Add that project as a Gitpod project
  3. Click on that project, get redirected to that project's page
  4. URL should be slugified.

Note: I do not have a GitHub app in my preview envs. If anyone could test it with theirs, please do. Nothing should change for GitHub projects.

Release Notes

New GitLab projects will have a slugified Project url

Documentation

@laushinka
Copy link
Contributor Author

laushinka commented Oct 24, 2021

I am currently on leave, but I can monitor this so I won't block the progress.

@AlexTugarev
Copy link
Member

AlexTugarev commented Oct 25, 2021

/werft run

👍 started the job as gitpod-build-laushinka-url-5847.14

@AlexTugarev AlexTugarev self-requested a review October 25, 2021 08:20
Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Hi @laushinka! Great to see this change happening! 🍰

issue: When I tried to add an existing GitLab project it didn't work as expected as you mentioned, however, I tried creating a new GitLab project and then adding it as a project in Gitpod but the same issue occurred.

issue: The same issue is also present on GitHub repositories on production. Is this PR also resolving this issue?

thought: Regarding the GH (GitHub) app, there's some relevant documentation (internal) which could help set up the app for this preview environment if you need.

@laushinka
Copy link
Contributor Author

laushinka commented Oct 26, 2021

/werft run

👍 started the job as gitpod-build-laushinka-url-5847.15

@laushinka
Copy link
Contributor Author

laushinka commented Nov 1, 2021

/werft run

👍 started the job as gitpod-build-laushinka-url-5847.16

@laushinka
Copy link
Contributor Author

laushinka commented Nov 1, 2021

Hi, @gtsiolis! Thanks for starting to review it 🙌🏽

I tried creating a new GitLab project and then adding it as a project in Gitpod but the same issue occurred.

Could you send screenshots or a recording? I tested it and it "worked on my machine" but I could be missing some steps or doing it incorrectly.

issue: The same issue is also present on GitHub repositories on production. Is this PR also resolving this issue?

Same as above, if you could send some screenshots or recording? I'm not aware that this bug exists for the GitHub ones, and just checked to confirm it too.

@laushinka
Copy link
Contributor Author

laushinka commented Nov 1, 2021

@gitpod-io/engineering-meta Just noticed that George is off, so if anyone would like to pick up the reviewing, that would be awesome 🤗

@laushinka
Copy link
Contributor Author

laushinka commented Nov 1, 2021

/werft run

👍 started the job as gitpod-build-laushinka-url-5847.18

if (!(await columnExists(queryRunner, "d_b_project", "slug"))) {
await queryRunner.query("ALTER TABLE d_b_project ADD COLUMN `slug` varchar(255) NULL");
}
if (await columnExists(queryRunner, "d_b_project", "slug")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do something for Gitlab repos here as well?

Copy link
Contributor Author

@laushinka laushinka Nov 2, 2021

Choose a reason for hiding this comment

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

You mean for the existing GitLab repos? That's the problem that I ran into, hence why this PR doesn't address existing GitLab repos. Existing GitLab ones have the wrong format in the name column, and we shouldn't populate the slug column with these. I'm not sure how best to populate existing ones with the correct slug. Ideas are very welcome 🙏🏽

Copy link
Contributor

Choose a reason for hiding this comment

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

#5847 was closed when this PR merged.

Regarding the strategy for #5847 (comment), my initial approach (perhaps naive) would have been to "lookup" the slug in GitLab by pulling in the repo details using the repo url. There would be a risk of collisions when we do that since I would expect that project slugs have to be unique within the team or user's project namespace.

Upon further reflection however, I'm inclined to go for option # 3

Leave the existing projects be. In the PR, existing GitLab projects will have NULL in the slug column, and the name column will be used instead, which is the current situation.

because:

  1. breaking URLs is not a good idea and
  2. I expect that we will end up offering a way to edit project slugs at some point in the future, and when we do that, should also offer redirect capabilities like people have come to expect (from GitHub).

@JanKoehnlein
Copy link
Contributor

/lgtm

@roboquat
Copy link
Contributor

roboquat commented Nov 2, 2021

LGTM label has been added.

Git tree hash: aed42b4825ea190de2be75fd7c977e8784457d6d

@roboquat
Copy link
Contributor

roboquat commented Nov 2, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JanKoehnlein

Associated issue: #5847

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project URL is not slugified for GitLab repos
6 participants