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

Allow markdown files to read from the LFS #5787

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Jan 20, 2019

This PR makes it possible for the markdown renderer to render images and media straight from the LFS.

Fix #5746

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath zeripath mentioned this pull request Jan 20, 2019
7 tasks
modules/lfs/utils.go Outdated Show resolved Hide resolved
@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 20, 2019
@codecov-io
Copy link

codecov-io commented Jan 20, 2019

Codecov Report

Merging #5787 into master will increase coverage by 0.03%.
The diff coverage is 42.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5787      +/-   ##
==========================================
+ Coverage   38.83%   38.87%   +0.03%     
==========================================
  Files         344      345       +1     
  Lines       49393    49491      +98     
==========================================
+ Hits        19184    19239      +55     
- Misses      27440    27471      +31     
- Partials     2769     2781      +12
Impacted Files Coverage Δ
modules/markup/markdown/markdown.go 73.14% <100%> (ø) ⬆️
routers/routes/routes.go 83.28% <100%> (+0.19%) ⬆️
routers/repo/view.go 46.22% <22.22%> (-1.09%) ⬇️
routers/repo/download.go 45.83% <30.95%> (-6.02%) ⬇️
modules/lfs/pointers.go 60.52% <60.52%> (ø)
modules/base/tool.go 75.2% <0%> (+1.07%) ⬆️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️
modules/lfs/content_store.go 53.12% <0%> (+1.56%) ⬆️

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 296814e...94bfc5d. Read the comment docs.

@bkcsoft bkcsoft 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 Jan 20, 2019
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Jan 20, 2019
@lafriks lafriks added this to the 1.8.0 milestone Jan 20, 2019
@zeripath
Copy link
Contributor Author

zeripath commented Jan 20, 2019

At present this is not a perfect implementation as it just redirects to the LFS Get which sets the content type as application/octet-stream and sets a filename to force a download. So /media URLs behave slightly different from /raw URLs. However it appears to solve the users issue Now /media actually serves the file and lets http set the content type.

@zeripath zeripath changed the title Allow markdown files to read from the LFS WIP: Allow markdown files to read from the LFS Jan 20, 2019
@zeripath
Copy link
Contributor Author

zeripath commented Jan 20, 2019

As there are no tests of this functionality at present I've marked this WIP until I've put some up one basic test implemented Have wired in tests in to TestGit. I think that should be enough.

routers/repo/download.go Outdated Show resolved Hide resolved
@zeripath zeripath force-pushed the issue-5746-allow-git-lfs-image-linking branch from c669d70 to b8ea638 Compare January 21, 2019 20:49
@zeripath
Copy link
Contributor Author

zeripath commented Jan 21, 2019

OK it appears that creating tests for this might be a bit more involved so I'm going to remove the WIP.

If proper integration tests are required please ask and I'll add some more.

More tests wired into TestGit .

@zeripath zeripath changed the title WIP: Allow markdown files to read from the LFS Allow markdown files to read from the LFS Jan 21, 2019
@lunny
Copy link
Member

lunny commented Jan 22, 2019

@zeripath please add integration test

@zeripath
Copy link
Contributor Author

Ok, basically the issue is that we don't have proper integration tests for LFS so I'll have to add those. This may broaden the scope the of this PR considerably as I suspect that a number of other issues will be found, for example, if we do any integration tests with gzip enabled then the issues that led to #5722 will manifest.

@techknowlogick
Copy link
Member

Perhaps a different PR is needed for the tests (at least all the ones that will be successful) which will at least reduce the potential increase in size of this PR

@zeripath
Copy link
Contributor Author

zeripath commented Jan 22, 2019

@techknowlogick I think that's the only sensible way to do it - I'm just loathe to make PR dependencies.

Our integration tests of the editor view are also somewhat lacking, as are those of the /raw endpoints and so on. This basically means that in order to get this 200 line change in, I need to write tests for at least 2000 lines of code.

I guess that's technical debt for you.

@zeripath
Copy link
Contributor Author

Of course if #5722 was merged I'd actually have the beginnings of an LFS test framework to work with...

@zeripath
Copy link
Contributor Author

OK the /raw and /media pathways are now checked in TestGit.

modules/lfs/utils.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor Author

I've removed the /wiki/media pathways as at present wiki files can't be placed in to the LFS and #5814 shows that a simple /wiki/media url route would need to reserve media from the wiki.

@zeripath
Copy link
Contributor Author

zeripath commented Feb 1, 2019

@lunny is the change of name to lfs_pointers.go ok?

@zeripath
Copy link
Contributor Author

zeripath commented Feb 7, 2019

Ping @lunny

@lunny
Copy link
Member

lunny commented Feb 7, 2019

@zeripath I think the file names could be more simpler as pointer.go and the methods could remove LFS since all these are on package lfs. When you use them, you will reference them as lfs.ReadLFSPointerFile so the duplicated lfs on method name is unnecessary.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath force-pushed the issue-5746-allow-git-lfs-image-linking branch from 82b96c8 to 6be64fb Compare February 7, 2019 16:59
@zeripath
Copy link
Contributor Author

zeripath commented Feb 7, 2019

OK, I've done that.

@zeripath
Copy link
Contributor Author

@lunny any more work needed on this?

@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 Feb 12, 2019
@zeripath zeripath merged commit 2a03e96 into go-gitea:master Feb 12, 2019
@zeripath zeripath deleted the issue-5746-allow-git-lfs-image-linking branch February 12, 2019 15:09
@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.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Git LFS image linking not possible
7 participants