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.

Sign up for GitHub

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

Jump to bottom

API to get single commit via SHA and Ref #10915

Merged
merged 12 commits into from
Apr 8, 2020
Merged

API to get single commit via SHA and Ref #10915

merged 12 commits into from
Apr 8, 2020

Conversation

6543
Copy link
Member

@6543 6543 commented Apr 1, 2020 •
edited

close #10089

GET /repos/:owner/:repo/commits/:ref

  • implement func
  • check ref/sha if it is one
  • write Tests

@6543 6543 changed the title [Feature] [API] Get a single commit [Feature] [API] Get a single commit va Ref Apr 1, 2020
@6543
Copy link
Member Author

6543 commented Apr 1, 2020

GH reference: https://developer.github.com/v3/repos/commits/#get-a-single-commit

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 1, 2020
@6543 6543 changed the title [Feature] [API] Get a single commit va Ref [API] Get a single commit va Ref Apr 1, 2020
@lunny lunny added the modifies/api This PR adds API routes or modifies them label Apr 1, 2020
@6543
Copy link
Member Author

6543 commented Apr 1, 2020

@lunny you could use GetSingleCommitBySHA for Ref too since the internal function doesnt care ...
but to make you API more similar to GitHub's one ... I created the api point

guillep2k
guillep2k reviewed Apr 1, 2020
View reviewed changes
routers/api/v1/repo/commits.go Outdated Show resolved Hide resolved
routers/api/v1/repo/commits.go Outdated Show resolved Hide resolved
@6543 6543 changed the title [API] Get a single commit va Ref [WIP] [API] Get a single commit va Ref Apr 1, 2020
@6543
Copy link
Member Author

6543 commented Apr 1, 2020 •
edited

@zeripath @lafriks @guillep2k so the summary of the discusion is ...

we can pass the string as ref as it is. (for GetSingleCommitByRef)

and if we restrict GetSingleCommitBySHA this will be a breaking change in API :(

@6543 6543 changed the title [WIP] [API] Get a single commit va Ref [API] Get a single commit va Ref Apr 1, 2020
@techknowlogick
Copy link
Member

This will aid with a Drone feature that is not implemented for Gitea: drone/go-scm#50

@6543 6543 changed the title [API] Get a single commit va Ref [API] Get a single commit via Ref Apr 2, 2020
@codecov-io
Copy link

codecov-io commented Apr 5, 2020 •
edited

Codecov Report

Merging #10915 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10915      +/-   ##
==========================================
+ Coverage   43.40%   43.42%   +0.01%     
==========================================
  Files         597      597              
  Lines       84527    84578      +51     
==========================================
+ Hits        36692    36725      +33     
- Misses      43311    43329      +18     
  Partials     4524     4524
Impacted Files Coverage Δ
modules/convert/convert.go 74.84% <ø> (-0.08%) ⬇️
modules/git/sha1.go 73.91% <ø> (ø)
modules/validation/binding.go 96.34% <100.00%> (+0.28%) ⬆️
routers/api/v1/api.go 76.37% <100.00%> (ø)
routers/api/v1/repo/commits.go 80.75% <100.00%> (+3.61%) ⬆️
models/repo_mirror.go 2.12% <0.00%> (-10.64%) ⬇️
services/pull/check.go 50.00% <0.00%> (-5.49%) ⬇️
services/pull/patch.go 62.93% <0.00%> (-2.80%) ⬇️
modules/git/command.go 86.95% <0.00%> (-2.61%) ⬇️
services/pull/temp_repo.go 29.05% <0.00%> (-2.57%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b252b23...9d85db4. Read the comment docs.

@guillep2k
Copy link
Member

I maintain my opinion that some sanitation should be performed 🙄. Commits are hex strings from 4 (min) to 40 (max) chars, all lower-case (regexp: ^[0-9a-f]{4,40}$). As for other references (in the other endpoint), I've listed a couple of cases that should be rejected as git itself rejected them when I tried them from the command line.

@techknowlogick techknowlogick added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Apr 5, 2020
@6543
Copy link
Member Author

6543 commented Apr 5, 2020

THIS is ready to review now

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 6, 2020
@6543
Copy link
Member Author

6543 commented Apr 6, 2020

CI fail looks unrelated

@techknowlogick can we add this to 1.12.0?

@techknowlogick techknowlogick added this to the 1.12.0 milestone Apr 6, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 6, 2020
@6543
Copy link
Member Author

6543 commented Apr 8, 2020

ready to merge 🚀

@techknowlogick techknowlogick merged commit 3d63caa into go-gitea:master Apr 8, 2020
@6543 6543 deleted the 10089-api-single-commit branch April 8, 2020 02:55
@6543 6543 mentioned this pull request Apr 8, 2020
Closed
@zeripath zeripath changed the title [API] Get a single commit via Ref API to get single commit via SHA Apr 8, 2020
@zeripath zeripath changed the title API to get single commit via SHA API to get single commit via SHA and Ref Apr 8, 2020
This was referenced May 11, 2020
Closed
Merged
@lafriks
Copy link
Member

lafriks commented May 11, 2020

Breaking functionality resolved in #11368

@lafriks lafriks removed the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label May 11, 2020
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
@6543
[API] Get a single commit via Ref (go-gitea#10915)
a308465 
* GET /repos/:owner/:repo/commits/:ref

* add Validation Checks

* Fix & Extend TEST

* add two new tast cases
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny left review comments

@mrsdizzie mrsdizzie mrsdizzie left review comments

@guillep2k guillep2k guillep2k left review comments

@lafriks lafriks lafriks approved these changes

@zeripath zeripath zeripath approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

[Feature] [API] Get a single commit
9 participants
@6543 @techknowlogick @codecov-io @guillep2k @lafriks @lunny @mrsdizzie @zeripath @GiteaBot