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

BUG: Gitlab Commits Before Date Needs More Logic #3193

Open
jimrobison opened this issue Jun 19, 2023 · 2 comments
Open

BUG: Gitlab Commits Before Date Needs More Logic #3193

jimrobison opened this issue Jun 19, 2023 · 2 comments
Labels
gitlab Issue related to Scorecard's GitLab client good first issue Good for newcomers kind/bug Something isn't working

Comments

@jimrobison
Copy link
Contributor

Describe the bug
Using the first commit returned has proven to not be the best source for the retrieval of merge requests details.

Expected behavior
The before field is intended to help limit the number of merge request details based on the last commit date for the default branch. By using the first record, this may not always include all of the related merge requests, throwing the scoring calculation off.

Additional context
I believe it would be better to calculate the date by cycling through the commits to determine the best date to use. I have a this is a working branch that I'm working through while validating the Code-Review check's scores

@jimrobison jimrobison added the kind/bug Something isn't working label Jun 19, 2023
@raghavkaul
Copy link
Contributor

raghavkaul commented Jun 29, 2023

By default, GitLab GraphQL returns the most recent merges on a GitLab repo. But we don't guarantee the merge request that brought in a commit on a GitLab repo's main branch is captured by our check - we may undercount - or miss - merge requests in zip if GraphQL and the REST API don't line up. (We use 1 GraphQL query + 1 REST API call instead of 1 REST API call per commit to find the linked Merge Request, which wouldn't scale).

The purpose of using mergedBefore in getMergeRequestDetail is to improve this - "don't give me any merge requests newer than the newest commit I've listed". But, I think you're right that before := commitsRaw[0].CommittedDate may not reflect the commit date of the newest commit (it might be much older, therefore leading to a lower score). We might fix this is to find a better bound commit date in commitsRaw, or by making a single API query to ListMergeRequestsByCommit to get the right date for before.

@raghavkaul raghavkaul added the gitlab Issue related to Scorecard's GitLab client label Jun 29, 2023
@LappleApple
Copy link

Bug affects scoring at the margins

@LappleApple LappleApple added the good first issue Good for newcomers label Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gitlab Issue related to Scorecard's GitLab client good first issue Good for newcomers kind/bug Something isn't working
Projects
Status: Backlog - Bugs
Development

No branches or pull requests

3 participants