-
Notifications
You must be signed in to change notification settings - Fork 497
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
✨ Gitlab support #2265
✨ Gitlab support #2265
Conversation
Integration tests success for |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2265 +/- ##
==========================================
- Coverage 44.85% 41.17% -3.68%
==========================================
Files 95 110 +15
Lines 7957 8774 +817
==========================================
+ Hits 3569 3613 +44
- Misses 4125 4892 +767
- Partials 263 269 +6 |
@N8BWert thank you so much for this PR! Very excited to see Scorecard support GitLab. I'm ooo today, will review it tomo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks amazing. Thanks for this PR.
A few things we can iterate on after merging are:
- Handling of tokens
- Adding e2e tests under the `./e2e/ folder.
- Scanning of critical repositories in the cron jobs (under
./cron
folder)
I'd suggest we add a "experimental feature" in the README until we have the e2e tests in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is great! Without tests, it is going to be hard to maintain this. Could you please add tests?
@naveensrinivasan which tests specifically? Under |
@naveensrinivasan By tests do you mean the e2e tests. If so, would you like me to host all of the e2e test repos on my gitlab or is there a plan to make a fork of those repos into an ossf specific GitLab? |
I think we should take the second option: have an ossf GitLab org |
Both TBH. I recommend we do the e2e and merge them in, as it provides some sanity. If not, we are blindsided whether any of these features are working. That is my thought process. |
If the e2e tests are in GitLab how do we ensure in this repository that new code changes aren't breaking this functionality? Do you have ideas? |
@N8BWert feel free to start the e2e tests using your own repos. In the meantime we'll figure out how to create an org |
Sounds good to me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this incredible PR @N8BWert! This looks good to submit overall. I left some comments, please address those and I can work on getting this merged in. My high-level comments on the PR is:
- Let's confine this PR to changes made within
clients/gitlabrepo/
package only. You can add the integrated support for GitLab in a future PR. - Remove any dead code that is still WIP and submit it in a future PR.
- Please add some unit tests for code where parsing/string building logic exists.
- There are some linter errors/warnings. Please fix those to adhere to our projects coding guidelines.
@azeemshaikh38 Sounds good, on it now. |
@laurentsimon @naveensrinivasan @N8BWert - regarding e2e: we might be biting off more than we can chew by attempting this all in a single PR. My suggestion would be to get this PR merged in with just code inside Note that |
I'm fine with adding e2etests later, see #2265 (comment) |
Ah I see. In that case, @naveensrinivasan do you still prefer having e2e tests? Note that this PR won't contain any logic outside |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
f593185
to
6b45307
Compare
…into gitlab_support
Signed-off-by: Nathaniel Wert <N8.Wert.B@gmail.com>
Integration tests success for |
Integration tests success for |
Integration tests success for |
* updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * merge main into gitlab_support * check-linter fixes Signed-off-by: Nathaniel Wert <N8.Wert.B@gmail.com> Co-authored-by: nathaniel.wert <nathaniel.wert@kudelskisecurity.com> Signed-off-by: N8BWert <n8.wert.b@gmail.com>
* updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * merge main into gitlab_support * check-linter fixes Signed-off-by: Nathaniel Wert <N8.Wert.B@gmail.com> Co-authored-by: nathaniel.wert <nathaniel.wert@kudelskisecurity.com>
* updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * merge main into gitlab_support * check-linter fixes Signed-off-by: Nathaniel Wert <N8.Wert.B@gmail.com> Co-authored-by: nathaniel.wert <nathaniel.wert@kudelskisecurity.com> Signed-off-by: N8BWert <n8.wert.b@gmail.com> Signed-off-by: nathaniel.wert <nathaniel.wert@kudelskisecurity.com>
* updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * merge main into gitlab_support * check-linter fixes Signed-off-by: Nathaniel Wert <N8.Wert.B@gmail.com> Co-authored-by: nathaniel.wert <nathaniel.wert@kudelskisecurity.com>
* updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * merge main into gitlab_support * check-linter fixes Signed-off-by: Nathaniel Wert <N8.Wert.B@gmail.com> Co-authored-by: nathaniel.wert <nathaniel.wert@kudelskisecurity.com> Signed-off-by: N8BWert <n8.wert.b@gmail.com> Signed-off-by: nathaniel.wert <nathaniel.wert@kudelskisecurity.com>
* updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * updated readme to reflect gitlab usage * bugfixes after a good deal of testing * removed unnecessary files from branch * cleaning up my mess * requested changes + unit tests * style fixes * merge main into gitlab_support * check-linter fixes Signed-off-by: Nathaniel Wert <N8.Wert.B@gmail.com> Co-authored-by: nathaniel.wert <nathaniel.wert@kudelskisecurity.com> Signed-off-by: nathaniel.wert <nathaniel.wert@kudelskisecurity.com>
What kind of change does this PR introduce?
This PR introduces GitLab functionality to the scorecard project.
What is the current behavior?
Currently scorecard only supports testing GitHub repositories.
What is the new behavior (if this is a feature change)?**
A client has been added to allow the command line execution of scorecard to include support for gitlab projects.
Which issue(s) this PR fixes
#Support For GitLab Projects
#2266 (comment)
Special notes for your reviewer
It is important to note that projectID is not the same as the name of the project. Scorecard will not find the GitLab project in the following situation:
Example: Project named test-project with projectID 1234 owned by steve who is a part of examplecompany
Does this PR introduce a user-facing change?
If a user is running scorecard on a GitHub repository that includes the string "gitlab." in the title scorecard will now think that repository is actually a GitLab project, however this is an incredibly rare edge case.