Skip to content

Conversation

ethantkoenig
Copy link
Member

@ethantkoenig ethantkoenig commented Aug 27, 2017

Fixes #2364. Modifies URL schemes to remove ambiguity (see #2364 (comment))

The GET /:owner/:repo/src/* endpoint becomes three endpoints:

  • GET /:owner/:repo/src/branch/*
  • GET /:owner/:repo/src/tag/*
  • GET /:owner/:repo/src/commit/*

Similarly for GET /:owner/:repo/raw/* and GET /:owner/:repo/commits/*. The old routes are redirected to the new routes to maintain backward compatibility.

@lunny lunny added the type/enhancement An improvement of existing functionality label Aug 28, 2017
@lunny lunny added this to the 1.x.x milestone Aug 28, 2017
@lunny lunny added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Aug 30, 2017
@jonasfranz
Copy link
Member

Is it possible to migrate links from older comments to the new URL scheme to avoid dead links?

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

@JonasFranzDEV I think a better solution to dead links is to have old links redirect to their new counterparts. I'll work on adding redirects soon.

@sapk
Copy link
Member

sapk commented Sep 5, 2017

~~~Add a paragraph inside swagger comment starting with "Deprecated:"~~~ 
@ethantkoenig
Copy link
Member Author

@sapk Isn't swagger just for API endpoints?

@lunny
Copy link
Member

lunny commented Sep 6, 2017

Yes. I think swagger only for API? This PR's break links is for UI.

@sapk
Copy link
Member

sapk commented Sep 6, 2017

@lunny @ethantkoenig sorry misread ...

@ethantkoenig ethantkoenig force-pushed the fix/ambiguous_url branch 2 times, most recently from f46f667 to 0230432 Compare September 10, 2017 00:15
@ethantkoenig
Copy link
Member Author

@JonasFranzDEV @sapk Added redirects from old URL scheme to new scheme

@codecov-io
Copy link

codecov-io commented Sep 15, 2017

Codecov Report

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

Impacted file tree graph

@@ Coverage Diff @@ ## master #2408 +/- ## ========================================== - Coverage 26.86% 26.85% -0.01%  ========================================== Files 89 89 Lines 17596 17601 +5 ========================================== Hits 4727 4727 - Misses 12183 12188 +5  Partials 686 686
Impacted Files Coverage Δ
models/webhook_slack.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 6e98812...865dd24. Read the comment docs.

@ethantkoenig
Copy link
Member Author

Rebased to resolve conflicts

@ethantkoenig ethantkoenig force-pushed the fix/ambiguous_url branch 2 times, most recently from d45690b to 1b94318 Compare September 22, 2017 04:25
@lafriks
Copy link
Member

lafriks commented Oct 18, 2017

@ethantkoenig can you please resolve conflicts so this can be tested?

@ethantkoenig
Copy link
Member Author

@lafriks Rebased

@lafriks
Copy link
Member

lafriks commented Oct 27, 2017

LGTM

@tboerger tboerger 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 Oct 27, 2017
@lafriks lafriks modified the milestones: 1.x.x, 1.3.0 Oct 27, 2017
@lunny
Copy link
Member

lunny commented Oct 27, 2017

cannot switch to another branch on the menus.

@ethantkoenig
Copy link
Member Author

@lunny Will take a look by Sunday at latest

@ethantkoenig
Copy link
Member Author

ethantkoenig commented Oct 29, 2017

@lunny Fixed. There were actually two problems:

  1. The branch dropdown template used the old URL scheme
  2. The redirect to the new URL scheme did not work

Both problems are fixed

@ethantkoenig
Copy link
Member Author

Rebasing to re-run CI (passes locally)

@lunny
Copy link
Member

lunny commented Oct 30, 2017

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 1 This PR needs approval from one additional maintainer to be merged. labels Oct 30, 2017
@lunny lunny merged commit 513375c into go-gitea:master Oct 30, 2017
@ethantkoenig ethantkoenig deleted the fix/ambiguous_url branch October 30, 2017 02:06
@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. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/enhancement An improvement of existing functionality

7 participants