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

Add API v3 routes for compitable with github #3076

Closed
wants to merge 1 commit into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 3, 2017

Copy all v1 routes to v3 currently.

@lunny lunny added modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality labels Dec 3, 2017
@lunny lunny added this to the 1.x.x milestone Dec 3, 2017
@codecov-io
Copy link

Codecov Report

Merging #3076 into master will decrease coverage by 0.01%.
The diff coverage is 98.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3076      +/-   ##
==========================================
- Coverage   33.46%   33.44%   -0.02%     
==========================================
  Files         270      270              
  Lines       39595    39601       +6     
==========================================
- Hits        13249    13245       -4     
- Misses      24460    24466       +6     
- Partials     1886     1890       +4
Impacted Files Coverage Δ
routers/api/v1/api.go 75.36% <98.7%> (+0.36%) ⬆️
models/repo_indexer.go 45.54% <0%> (-3.47%) ⬇️
models/repo.go 38.15% <0%> (-0.19%) ⬇️

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 ab62da2...b77a17f. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 3, 2017
@lafriks
Copy link
Member

lafriks commented Dec 3, 2017

Can you add in the way that only one can be changed?

@lunny
Copy link
Member Author

lunny commented Dec 3, 2017

@lafriks they point the same route function currently. So you will always change both.

@lafriks
Copy link
Member

lafriks commented Dec 3, 2017

@lunny there is PR to change api route name to match github (#2639) so it would be nice to be able to do that only for v3 and for v1 leave as it is to not break anything just in case

}, context.APIContexter())

m.Group("/v3", func() {
apiRoutes(m)
Copy link
Member

Choose a reason for hiding this comment

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

Some of these routes are not GitHub-compatible. They shouldn't be added to v3/ 😕

@lunny
Copy link
Member Author

lunny commented Dec 4, 2017

OK. I think we could add new route /v3 and only move github api v3 comiptable routes there.

@sapk
Copy link
Member

sapk commented Dec 4, 2017

It would be best to also separate the route into sub-route to not have a "big file" like v1.

// RegisterRoutes registers all v1 APIs routes to web application.
// FIXME: custom form error response
func RegisterRoutes(m *macaron.Macaron) {
func apiRoutes(m *macaron.Macaron) {
Copy link
Member

Choose a reason for hiding this comment

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

I would add a ghCompat bool argument here, and if !ghCompat { ... } around any custom endpoints we have

@stale
Copy link

stale bot commented Jan 5, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Jan 5, 2019
@techknowlogick
Copy link
Member

Closing per above conversation, we can open a fresh PR that includes only routes compatible with GitHub v3

@lafriks lafriks removed this from the 1.x.x milestone Jan 24, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@lunny lunny deleted the lunny/api_compitable_github branch August 24, 2023 11:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/stale lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants