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

Enable caching on assets and avatars #3376

Merged
merged 6 commits into from
Feb 3, 2018
Merged

Enable caching on assets and avatars #3376

merged 6 commits into from
Feb 3, 2018

Conversation

thehowl
Copy link
Contributor

@thehowl thehowl commented Jan 15, 2018

fix #3323. Also, fixed a bug where whenever an avatar was uploaded but the user didn't reset the AvatarEmail field in the settings page, the hash would be set to the email and the avatar would be uploaded there (to the address of the hash of the email).

return opts.staticHandler(dir)
}

func (opts *Options) staticHandler(dir string) macaron.Handler {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of staticHandler has been copied over from parts of macaron. The handle function was modified to use ExpireAfter instead of the Expires function, and also now it has proper support for ETags (by reading the req header If-None-Match).

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 15, 2018
@codecov-io
Copy link

codecov-io commented Jan 15, 2018

Codecov Report

Merging #3376 into master will decrease coverage by 0.01%.
The diff coverage is 76.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3376      +/-   ##
==========================================
- Coverage   35.64%   35.63%   -0.02%     
==========================================
  Files         281      281              
  Lines       40586    40662      +76     
==========================================
+ Hits        14467    14489      +22     
- Misses      23982    24028      +46     
- Partials     2137     2145       +8
Impacted Files Coverage Δ
models/user.go 39.43% <0%> (-0.33%) ⬇️
routers/routes/routes.go 87.58% <100%> (+0.04%) ⬆️
modules/public/dynamic.go 100% <100%> (ø) ⬆️
modules/public/public.go 74.41% <74.11%> (-25.59%) ⬇️
models/repo_indexer.go 41.15% <0%> (-7.53%) ⬇️
modules/indexer/indexer.go 70% <0%> (-7.5%) ⬇️
modules/indexer/repo.go 60.86% <0%> (-6.96%) ⬇️
models/repo.go 42.98% <0%> (-0.19%) ⬇️
models/repo_list.go 67.18% <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 77f8bad...79fadd1. Read the comment docs.

defer f.Close()

fi, err := f.Stat()
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

It should at least log error in such case

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Jan 15, 2018
@lafriks
Copy link
Member

lafriks commented Jan 16, 2018

@thehowl there is go fmt problem code

@thehowl
Copy link
Contributor Author

thehowl commented Jan 17, 2018

@lafriks can you restart drone?

@lunny
Copy link
Member

lunny commented Jan 18, 2018

restarted

@thehowl
Copy link
Contributor Author

thehowl commented Jan 18, 2018

Tests are green. Please review! 🙏 (Can we get this in by 1.4?)

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

LGTM

One nit is that the avatars expire after one hour, should that be longer? The other static route above is set to 6 hours, could that be the same for avatars? Feel free to disregard this nit as the time can be changed in a diff PR if needed.

@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 Jan 20, 2018
@thehowl
Copy link
Contributor Author

thehowl commented Jan 20, 2018

One nit is that the avatars expire after one hour, should that be longer?

Actually, yes, that sounds very reasonable. The reason why they were originally an hour is that I didn't know whether avatar URLs were changed when they were. But since they do, it's reasonable to change that.

By the way, this will be merged in 1.5.0, so for other maintainers: no hurry to review this

@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 Feb 3, 2018
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Feb 3, 2018
@lafriks lafriks merged commit 17655cd into go-gitea:master Feb 3, 2018
@thehowl thehowl deleted the fix-3323 branch April 22, 2018 20:02
aswild added a commit to aswild/gitea that referenced this pull request Jul 6, 2018
* SECURITY
  * Limit uploaded avatar image-size to 4096x3072 by default (go-gitea#4353)
  * Do not allow to reuse TOTP passcode (go-gitea#3878)
* FEATURE
  * Add cli commands to regen hooks & keys (go-gitea#3979)
  * Add support for FIDO U2F (go-gitea#3971)
  * Added user language setting (go-gitea#3875)
  * LDAP Public SSH Keys synchronization (go-gitea#1844)
  * Add topic support (go-gitea#3711)
  * Multiple assignees (go-gitea#3705)
  * Add protected branch whitelists for merging (go-gitea#3689)
  * Global code search support (go-gitea#3664)
  * Add label descriptions (go-gitea#3662)
  * Add issue search via API (go-gitea#3612)
  * Add repository setting to enable/disable health checks (go-gitea#3607)
  * Emoji Autocomplete (go-gitea#3433)
  * Implements generator cli for secrets (go-gitea#3531)
* ENHANCEMENT
  * Add more webhooks support and refactor webhook templates directory (go-gitea#3929)
  * Add new option to allow only OAuth2/OpenID user registration (go-gitea#3910)
  * Add option to use paged LDAP search when synchronizing users (go-gitea#3895)
  * Symlink icons (go-gitea#1416)
  * Improve release page UI (go-gitea#3693)
  * Add admin dashboard option to run health checks (go-gitea#3606)
  * Add branch link in branch list (go-gitea#3576)
  * Reduce sql query times in retrieveFeeds (go-gitea#3547)
  * Option to enable or disable swagger endpoints (go-gitea#3502)
  * Add missing licenses (go-gitea#3497)
  * Reduce repo indexer disk usage (go-gitea#3452)
  * Enable caching on assets and avatars (go-gitea#3376)
  * Add repository search ordered by stars/forks. Forks column in admin repo list (go-gitea#3969)
  * Add Environment Variables to Docker template (go-gitea#4012)
  * LFS: make HTTP auth period configurable (go-gitea#4035)
  * Add config path as an optionial flag when changing pass via CLI (go-gitea#4184)
  * Refactor User Settings sections (go-gitea#3900)
  * Allow square brackets in external issue patterns (go-gitea#3408)
  * Add Attachment API (go-gitea#3478)
  * Add EnableTimetracking option to app settings (go-gitea#3719)
  * Add config option to enable or disable log executed SQL (go-gitea#3726)
  * Shows total tracked time in issue and milestone list (go-gitea#3341)
* TRANSLATION
  * Improve English grammar and consistency (go-gitea#3614)
* DEPLOYMENT
  * Allow Gitea to run as different USER in Docker (go-gitea#3961)
  * Provide compressed release binaries (go-gitea#3991)
  * Sign release binaries (go-gitea#4188)
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
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.

Avatars uploaded to instance are not cached
7 participants