Skip to content

Conversation

sapk
Copy link
Member

@sapk sapk commented Oct 19, 2017

Fix #2745 & update swagger-ui version

@lunny lunny added this to the 1.x.x milestone Oct 20, 2017
@lunny lunny added the type/enhancement An improvement of existing functionality label Oct 20, 2017
@thehowl
Copy link
Contributor

thehowl commented Oct 20, 2017

As I said in #2745, for backwards compatibility I'd say it's best to redirect users to /api/swagger when they try to access the org or user on /swagger, and only show the org or user when there actually is one.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 20, 2017
@sapk
Copy link
Member Author

sapk commented Oct 20, 2017

I don't think that it is really usefull to keep a backward compatibility on documentation link. The JSON containing the API specification that could be used by other system hasn't move.

Before merging, I forgot to update the link in the footer template.

@thehowl
Copy link
Contributor

thehowl commented Oct 20, 2017

yeah, that's true, however my concern is that this pr will effectively 404 all of the current links to /swagger around the web

@sapk
Copy link
Member Author

sapk commented Oct 20, 2017

@thehowl there should be only in the footer inside gitea itself. I will update other docs links like https://github.com/go-gitea/docs/blob/adef669bcc10c63300692839eb1b57f001777ee2/config.yaml#L34

@codecov-io
Copy link

codecov-io commented Oct 20, 2017

Codecov Report

Merging #2746 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #2746 +/- ## ========================================= - Coverage 26.91% 26.9% -0.01%  ========================================= Files 87 88 +1 Lines 17286 17289 +3 ========================================= Hits 4652 4652 - Misses 11955 11958 +3  Partials 679 679
Impacted Files Coverage Δ
routers/api/v1/misc/swagger.go 0% <0%> (ø)

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 bc8d726...f857d38. Read the comment docs.

@strk
Copy link
Member

strk commented Oct 20, 2017

Is public/vendor to be managed manually like in this PR or shouldn't there be a vendoring tool configuration updated ? Other than that, here's my trusted LGTM

@lafriks
Copy link
Member

lafriks commented Oct 20, 2017

@strk yes public vendor is currently managed manually.

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 20, 2017
@sapk
Copy link
Member Author

sapk commented Oct 20, 2017

@strk I have added the task in the makefile for easing the process but it is better to keep it under manual update since there is also a template (serve under /api(/v1)/swagger) manually derivated from index.html to work under that url. Doing it manually also ensure that the specs at still working with the swagger-ui display.

For information, You can also open the swagger-ui interface directly via /vendor/assets/swagger-ui/index.html and it will load the specs but that's not a pretty url ^^.

@lafriks
Copy link
Member

lafriks commented Oct 21, 2017

Make LG-TM work again

@lunny lunny modified the milestones: 1.x.x, 1.3.0 Oct 21, 2017
@lunny lunny merged commit 619b9b5 into go-gitea:master Oct 21, 2017
@sapk sapk deleted the move-swagger-url branch October 22, 2017 00:11
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality

7 participants