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

[Depfu] Add support for Gitlab #7475

Merged
merged 3 commits into from
Jan 29, 2022
Merged

Conversation

kris7t
Copy link
Contributor

@kris7t kris7t commented Jan 11, 2022

Fixes #7474

Following the recommendation #7474 (comment) of @chris48s, this PR adds a :vcsType parameter to depfu URLs to select between Github and Gitlab. Badges without a VCS type are redirected to the Github route via a legacy route.

@shields-ci
Copy link

shields-ci commented Jan 11, 2022

Messages
📖 ✨ Thanks for your contribution to Shields, @kris7t!

Generated by 🚫 dangerJS against 607c509

@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Jan 11, 2022
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7475 January 11, 2022 01:36 Inactive
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thanks! Think we could use another test but otherwise LGTM

@chris48s maybe not worth discussing as part of this PR since the route change is both consistent with what we had before as well as other dependency management/update services, but it seems like this class of badge routes are all missing the NOUN portion from our canonical badge url pattern

services/depfu/depfu.tester.js Show resolved Hide resolved
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7475 January 11, 2022 02:01 Inactive
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

Nice. Thanks for having a look at this and providing tests.

static route = { base: 'depfu', pattern: ':user/:repo' }
static route = {
base: 'depfu',
pattern: ':vcsType(github|gitlab)/:user/:repo',
Copy link
Member

Choose a reason for hiding this comment

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

With GitLab we can't just use :user/:repo, repos can be nested in an aribtrary number of nested subgroups e.g:
https://gitlab.com/megabyte-labs/docker/ci-pipeline/ansible-lint so for projects that work with gitlab we would usually make the route something like :vcsType(github|gitlab)/:project+ (which allows an arbitrary number of slashes) and then call https://depfu.com/${vcsType}/shields/${project} assuming depfu inherits that design descension and exposes it in the same way. Do we have an example of this?

@chris48s
Copy link
Member

maybe not worth discussing as part of this PR since the route change is both consistent with what we had before as well as other dependency management/update services, but it seems like this class of badge routes are all missing the NOUN portion from our canonical badge url pattern

@calebcartwright I think if we want to change it to /depfu/dependencies/(github|gitlab)/:project+ we might as well do it now in this PR. Then we only need to add one redirect from /depfu/:user/:repo to /depfu/dependencies/github/:project+. If we ship it with /depfu/:vcsType(github|gitlab)/:project+ and then change it in another PR we need another redirect to avoid a BC break. Lets not do that if we don't need to. That said, we're doing the same thing on requires.io and libraries.io so I can see a case for just ignoring it.

@kris7t
Copy link
Contributor Author

kris7t commented Jan 11, 2022

I just created an example project to test this. It might be a bit trickier, because Depfu seems to require urlencoded subgroup separators: https://depfu.com/gitlab/shields/shields-example-group%252Fsubgroup/example-nodejs (replacing %252F with / doesn't work).

I guess the best approach would be to parse the :project+ argument, and replace all but the last / characters with %252F?

@chris48s
Copy link
Member

Lets use encodeURIComponent() instead of manually replacing.

For the Gitlab case we can do something like

encodeURIComponent(project.substr(0, project.lastIndexOf('/')))
project.substr(project.lastIndexOf('/'))

We'll also want to make sure project has at least one / in it before doing that, so something like

if (!project.includes('/')) {
  throw InvalidParameter()
}

class InvalidParameter extends ShieldsRuntimeError {

For the Github case, we can just pass project as-is

@kris7t
Copy link
Contributor Author

kris7t commented Jan 25, 2022

I updated this PR according to the review comments:

  • The endpoint was moved to /depfu/dependencies.
  • If there is no / in the project, we throw an InvalidParameter error. I left it with a default message, because I couldn't come up with any specific prettyMessage that wasn't way too long.
  • I added the URI encoding regardless the vcsType to simplify logic (it doesn't do anything for Github, but it won't hurt either).
  • For testing, I use the example group, subgroup, and repo that I created on my Gitlab account (the test nested repo has to have Depfu enabled, but I couldn't find any existing example repo that was nested in a subgroup).

@kris7t kris7t requested a review from chris48s January 25, 2022 00:51
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

Great - thanks for finishing this up 👍 Sorry it took a few days to get back to it

@repo-ranger repo-ranger bot merged commit 5b29290 into badges:master Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Depfu for Gitlab repos
5 participants