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

[GitLabCoverage] subgroup bug #8401

Merged
merged 16 commits into from
Sep 28, 2022

Conversation

PaulaBarszcz
Copy link
Collaborator

@PaulaBarszcz PaulaBarszcz commented Sep 15, 2022

@shields-ci
Copy link

shields-ci commented Sep 15, 2022

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

Generated by 🚫 dangerJS against 78a5e9a

@PaulaBarszcz
Copy link
Collaborator Author

To accomodate GitLab subgroups I need to allow for multiple slashes in the last namedParam (named project).
Thus the branch needs to become a queryParam.

While writing GitlabCoverageRedirector I ran into an issue:
http://localhost:8080/gitlab/coverage/gitlab-org/gitlab-runner/master
Here the redirector does not kick in, since it assumes that master above is a part of the project namedParam.

Could I rename the new endpoint to e.g. gitlab/coverage-v2 , gitlab/coverage-subgroups or sth else?
Or is some other solution preferable?

@calebcartwright calebcartwright added the service-badge New or updated service badge label Sep 18, 2022
@chris48s
Copy link
Member

As is often the case, the hardest part is giving things a good name :) This is basically the same issue we hit when we made the same change on the gitlab pipelines build status badge, which is why the old route was /pipeline and the new one is /pipeline-status.

A couple of options I considered were /test-coverage and /cov but I think on balance the best option here would actually be to make the new route /pipeline-coverage as really this badge is a coverage badge for GitLab Pipelines. Then those 2 routes fit quite nicely.

@PaulaBarszcz
Copy link
Collaborator Author

PaulaBarszcz commented Sep 25, 2022

EDIT:
Below I pasted the response from @chris48s from Discord that helped me with running GitLab service tests locally:

to run with a gitlab token, I think you'll need this in your local.yml

private:
  # any other local tokens
  gitlab_token: your-token-here
public:
  services:
    gitlab:
      authorizedOrigins: 'https://gitlab.com'

Hi @chris48s,
When you're free, could you please take a look why is the redirection test failing in services/gitlab/gitlab-coverage.tester.js?

Should I add a GitLab secret to be able to run GitLab service tests locally? I generated a GitLab secret and added it to my local.yml file but GitLab service tests still don’t pass.
It looks that also not-locally there is an issue with credentials.

Running this locally on my machine currently results in 73 tests failing (both with gitlab_token filled with my personal access token and still with the ... value).
npm run test:services -- --only="gitlab"
Zrzut ekranu 2022-09-25 o 17 45 23

https://app.circleci.com/pipelines/github/badges/shields/14993/workflows/345e50a3-3953-459f-a62e-bbfdc4773176/jobs/196306
Zrzut ekranu 2022-09-25 o 17 39 36

@PaulaBarszcz PaulaBarszcz changed the title [GitLab] coverage subgroup bug [GitLabCoverage] subgroup bug Sep 26, 2022
@PaulaBarszcz PaulaBarszcz marked this pull request as ready for review September 26, 2022 20:25
@PaulaBarszcz
Copy link
Collaborator Author

This PR is ready for review.

@@ -3,59 +3,63 @@ import { createServiceTester } from '../tester.js'
export const t = await createServiceTester()
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a couple of test cases in here for the redirector: one with a branch and one without. There is a test helper .expectRedirect() you can use to make sure the correct redirect was issued.

Copy link
Collaborator Author

@PaulaBarszcz PaulaBarszcz Sep 28, 2022

Choose a reason for hiding this comment

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

I renamed gitlab-coverage.service to gitlab-pipeline-coverage.service
to reflect the change in the URL path.

I added gitlab-coverage-redirect.service (similar as in gitlab-license-redirect.service). In this file, I placed tests with redirector (branch+job_name/gitlab_url).

Since branch was a required param in the previous gitlab coverage route, we cannot add a test without it to test the redirection ;)
image

@PaulaBarszcz
Copy link
Collaborator Author

This PR is ready for a re-review.

{
title:
'Gitlab code coverage (with a subgroup in the name of the project)',
namedParams: { project: 'megabyte-labs/go/cli/bodega' },
Copy link
Member

Choose a reason for hiding this comment

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

Nice. One more minor thing. We can drop this example. For all the other GitLab badges (pipeline status, issues, merge requests, releases, etc) they work for repos in nested subgroups but we don't explicitly provide an example in the frontend. In general we try and keep the number of example of different variations in the examples to a minimum if possible. Cheers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, example removed ;)

@chris48s chris48s merged commit 2260126 into badges:master Sep 28, 2022
@chris48s
Copy link
Member

sorted - cheers 👍

@PaulaBarszcz PaulaBarszcz deleted the gitlab-coverage-subgroup-bug branch September 28, 2022 20:09
@PaulaBarszcz
Copy link
Collaborator Author

sorted - cheers 👍

Great, thanks @chris48s ;)
When you're free, please feel free to close the related issue: #7473

@chris48s
Copy link
Member

oh yeah. Tip: If you put "closes #[issue num]" in the top post, the issue will auto-close when the PR is merged
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants