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

[GithubLastCommit] [GitlabLastCommit] [GiteaLastCommit] Support file path for last commit #10041

Merged
merged 10 commits into from Mar 25, 2024

Conversation

ReubenFrankel
Copy link
Contributor

@ReubenFrankel ReubenFrankel commented Mar 22, 2024

Closes #7351

Adds support to resolve last commit for a given file path, for GitHub, GitLab and Gitea services.

Copy link
Contributor

github-actions bot commented Mar 22, 2024

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

Generated by 🚫 dangerJS against 4d9a24c

@ReubenFrankel ReubenFrankel changed the title [GithubLastCommit] Support file path for last commit [GithubLastCommit] [GitlabLastCommit] [GiteaLastCommit] Support file path for last commit Mar 23, 2024
@ReubenFrankel ReubenFrankel marked this pull request as ready for review March 23, 2024 01:25
@ReubenFrankel ReubenFrankel reopened this Mar 23, 2024
@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label Mar 24, 2024
Copy link
Contributor

🚀 Updated review app: https://pr-10041-badges-shields.fly.dev

@chris48s
Copy link
Member

Thanks. This looks good, but I think we could do with improving the error handling/messages slightly.

On the GitLab and GitHub APIs, if we specify a path that doesn't exist, we get back an empty array. This means the response fails schema validation.
Tbh, I suspect this probably also happens for a repo with zero commits, but this is not something that has come up before.
It would be nice to throw a more informative error in this cases. My suggestion would be we remove the .min(1) requirement from the schema and then after we fetch we throw a NotFound('no commits found') if there are zero items in the array (to cover both situations).

With the Gitea API, if the path doesn't exist, we get a 404 back and throw the error "user or repo not found". I think for this case, we can just amend the error message to "user, repo or path not found"

We should also have test cases for the errors.

ReubenFrankel and others added 2 commits March 24, 2024 19:00
Co-authored-by: chris48s <chris48s@users.noreply.github.com>
@chris48s
Copy link
Member

Just looking over your other similar PR for bitbucket, you've already adopted the same pattern I described for the error handling there 👍

@ReubenFrankel
Copy link
Contributor Author

@chris48s I've made the suggested changes - tests are passing locally so not sure why they are failing in CI (can't see from the log).

Copy link
Contributor

🚀 Updated review app: https://pr-10041-badges-shields.fly.dev

@chris48s
Copy link
Member

Thanks. This looks good.

Looks like the test failures were just a transient failure with some requests timing out.

Merged via the queue into badges:master with commit 4f141df Mar 25, 2024
38 of 40 checks passed
@ReubenFrankel ReubenFrankel deleted the 7351-last-commit-path branch March 25, 2024 12:56
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.

Last Commit based on File/Folder in repo
2 participants