Skip to content

[stable10] Allow configuring thumbnails path#35131

Merged
PVince81 merged 2 commits intostable10from
stable10-configurable-thumbnails-path
May 10, 2019
Merged

[stable10] Allow configuring thumbnails path#35131
PVince81 merged 2 commits intostable10from
stable10-configurable-thumbnails-path

Conversation

@PVince81
Copy link
Copy Markdown
Contributor

@PVince81 PVince81 commented Apr 30, 2019

Similar to "cache_path" but for thumbnails.
This was quickly copied from the CacheMountProvider

Why ? Because I tried to set this up on my server with symlinks and it says "following symlinks not allowed".

@PVince81 PVince81 added this to the development milestone Apr 30, 2019
@PVince81 PVince81 requested a review from DeepDiver1975 April 30, 2019 19:03
@PVince81 PVince81 self-assigned this Apr 30, 2019
@PVince81
Copy link
Copy Markdown
Contributor Author

PVince81 commented Apr 30, 2019

One reason I want a separate thumbnails folder is because I consider this like a cache and don't want this in backups. Also having it separate makes it easier to clean up.

@PVince81
Copy link
Copy Markdown
Contributor Author

PVince81 commented Apr 30, 2019

Tested on my own server by patching on top of 10.2.0 RC1, works fine.

My thumbnails are stored in "/var/cache/thumbnails/$user" now.

@PVince81 PVince81 requested a review from mmattel April 30, 2019 19:06
@PVince81 PVince81 changed the title [stable10] Allow configuring thumbnails patAllow configuring thumbnails pathh [stable10] Allow configuring thumbnails path Apr 30, 2019
mmattel
mmattel reviewed Apr 30, 2019
View reviewed changes
@PVince81
Copy link
Copy Markdown
Contributor Author

PVince81 commented May 2, 2019 •
edited
Loading

  • update doc
  • add unit tests for both the cache mount provider and the new one

@PVince81 PVince81 force-pushed the stable10-configurable-thumbnails-path branch from f240bc1 to 2dadf07 Compare May 7, 2019 12:49
@codecov
Copy link
Copy Markdown

codecov bot commented May 7, 2019

Codecov Report

Merging #35131 into stable10 will decrease coverage by 19.5%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             stable10   #35131       +/-   ##
===============================================
- Coverage       64.47%   44.97%   -19.51%     
===============================================
  Files            1288      116     -1172     
  Lines           76936    11565    -65371     
  Branches         1307     1307               
===============================================
- Hits            49606     5201    -44405     
+ Misses          26949     5983    -20966     
  Partials          381      381
Flag Coverage Δ Complexity Δ
#javascript 53.01% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 30.71% <ø> (-34.99%) 0 <ø> (-20070)
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Storage/DAV.php 59.45% <0%> (-21.64%) 0% <0%> (ø)
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
apps/files/lib/AppInfo/Application.php
apps/systemtags/list.php
... and 1163 more

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 ee5f45e...2dadf07. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov bot commented May 7, 2019 •
edited
Loading

Codecov Report

Merging #35131 into stable10 will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##             stable10   #35131      +/-   ##
==============================================
+ Coverage       64.48%   64.49%   +<.01%     
- Complexity      20074    20078       +4     
==============================================
  Files            1288     1289       +1     
  Lines           76955    76966      +11     
  Branches         1307     1307              
==============================================
+ Hits            49626    49637      +11     
  Misses          26948    26948              
  Partials          381      381
Flag Coverage Δ Complexity Δ
#javascript 53.01% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.71% <100%> (ø) 20078 <4> (+4) ⬆️
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Mount/PreviewsMountProvider.php 100% <100%> (ø) 4 <4> (?)
lib/private/Server.php 86.56% <100%> (+0.01%) 253 <0> (ø) ⬇️
lib/private/Files/Mount/CacheMountProvider.php 100% <100%> (ø) 4 <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 6b4e0e9...c57b8b7. Read the comment docs.

@PVince81
Copy link
Copy Markdown
Contributor Author

PVince81 commented May 7, 2019

@mmattel I've adjusted the doc, please check.

I've now written unit tests for both the previews mount provider and cache mount provider.
Apparently the "$loader" wasn't properly passed in.

@DeepDiver1975 please review

DeepDiver1975
DeepDiver1975 approved these changes May 7, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Just one minor phpdoc issue - 👍

lib/private/Files/Mount/PreviewsMountProvider.php Outdated
private $config;

/**
* ObjectStoreHomeMountProvider constructor.
Copy link
Copy Markdown
Member

@DeepDiver1975 DeepDiver1975 May 7, 2019

Choose a reason for hiding this comment

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

ObjectStore ❓

Copy link
Copy Markdown
Contributor Author

@PVince81 PVince81 May 7, 2019

Choose a reason for hiding this comment

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

fixed now in both cache and preview providers

@PVince81 PVince81 force-pushed the stable10-configurable-thumbnails-path branch from 2dadf07 to 223e83c Compare May 7, 2019 13:04
mmattel
mmattel reviewed May 7, 2019
View reviewed changes
config/config.sample.php Outdated
* and `$user` will be substituted with the user id automatically.
*
* For example if "previews_path" is "/var/cache/owncloud/thumbnails" then for a logged in
* user "user1" the thumbnail path will be "/var/cache/owncloud/thumbnails/user1".
Copy link
Copy Markdown
Contributor

@mmattel mmattel May 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can you pls use back-ticks instead quotes
As you correctly do in the block above...

The text is fine, thanks for adding it 👍

@PVince81 PVince81 force-pushed the stable10-configurable-thumbnails-path branch from 223e83c to c57b8b7 Compare May 9, 2019 14:30
@PVince81
Copy link
Copy Markdown
Contributor Author

PVince81 commented May 9, 2019

all adjusted, including CI issues

@PVince81 PVince81 merged commit 1e67d80 into stable10 May 10, 2019
@PVince81 PVince81 deleted the stable10-configurable-thumbnails-path branch May 10, 2019 06:58
@PVince81 PVince81 mentioned this pull request May 10, 2019
Merged
@PVince81
Copy link
Copy Markdown
Contributor Author

PVince81 commented May 10, 2019

master: #35196

@davitol davitol mentioned this pull request Sep 3, 2019
Closed
13 tasks
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DeepDiver1975 DeepDiver1975 DeepDiver1975 approved these changes

@mmattel mmattel mmattel approved these changes

Assignees

@PVince81 PVince81

Labels

4 - To release

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

6 participants

@PVince81 @DeepDiver1975 @mmattel @phil-davis @micbar @patrickjahns