Project

General

Profile

Actions

Story #8784

closed

[Workbench] Use keep-web to generate directory listings

Added by Tom Clegg over 6 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Start date:
03/23/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

Description

Human- and machine-readable directory listings would allow browsing collections without involving Workbench.

TBD: Bookmarks to non-public collections in keep-web come with a bit of a snag, which auto-indexing could turn into a bigger practical problem. If you click a bookmark/permalink instead of going through Workbench's links/redirector, and you don't already have an active cookie from previously clicking the Workbench links, there's no option to log in -- you just get an error. You have to recognize the error yourself, find the collection in Workbench, and follow a link from there.

(Features copied from #5824's "nice to have" section)
  • Very basic formatting (bootstrap would be nice but can wait)
  • OK if we show a directory listing only at the top level, similar to the way Workbench does it.
  • Explicitly no index.html in place of directory listings.
    • This means "wget -r" can always be expected to work, but it also ensures this can't be used to host entire web sites along the lines of github pages. If we add this capability in the future we could add a -render-index flag. This would make more sense if it came with a bunch of other features anyway, like dynamically mapping vhosts to collections.

A complementary piece to this is having Workbench generate sharing links which point directly to keep-web rather than redirecting through Workbench.

This is in support of a request to be able to share data without exposing the Workbench host. https://support.curoverse.com/rt/Ticket/Display.html?id=364


Files

index.page.2dc0e00.png (66.6 KB) index.page.2dc0e00.png Tom Clegg, 06/13/2017 01:00 PM
index.page.0e3369b.png (79.8 KB) index.page.0e3369b.png Tom Clegg, 06/13/2017 01:00 PM
index.page.a42cb73.png (67.3 KB) index.page.a42cb73.png Tom Clegg, 06/13/2017 02:00 PM

Subtasks 3 (0 open3 closed)

Task #11887: Review 8784-dir-listingsResolvedPeter Amstutz03/23/2016

Actions
Task #11872: Workbench redirectResolvedTom Clegg03/23/2016

Actions
Task #11834: Review 8784-dir-listingsResolvedTom Clegg03/23/2016

Actions

Related issues

Related to Arvados - Story #5824: [Workbench] [Keep] collection browse/download serverResolvedTom Clegg05/21/2015

Actions
Related to Arvados - Story #11167: [Workbench] Remove arv-get file download fallbackResolvedLucas Di Pentima07/24/2017

Actions
Actions #1

Updated by Brett Smith over 6 years ago

  • Target version set to Arvados Future Sprints
Actions #2

Updated by Peter Amstutz about 5 years ago

Related to this, workbench sharing links should link directly to the keep-web index page.

Actions #3

Updated by Tom Morris about 5 years ago

  • Target version changed from Arvados Future Sprints to 2017-06-21 sprint
Actions #4

Updated by Tom Morris about 5 years ago

  • Description updated (diff)
  • Story points set to 2.0
Actions #5

Updated by Tom Morris about 5 years ago

  • Tracker changed from Bug to Story
Actions #6

Updated by Tom Clegg about 5 years ago

  • Assigned To set to Tom Clegg
Actions #7

Updated by Tom Clegg about 5 years ago

  • Status changed from New to In Progress
Actions #8

Updated by Tom Clegg about 5 years ago

  • File index page 0e3369b.png added
  • File index page 2dc0e00.png added
Actions #9

Updated by Tom Clegg about 5 years ago

Existing Workbench index page

new keep-web index page @ 2dc0e00

TODO:
  • use new SDK to load into collection record and cache that, instead of loading/caching a map[string]interface and copying to a Collection in each request
TBD:
  • (separate branch) redirect Workbench index page URL to keep-web, and use keep-web directly when generating new links
  • monospace font change a good idea?
  • dropping the hierarchy a good idea? if so, maybe only the name should be hotlinked, not the whole path?
  • rename (*Collection)FileSystem() to ROFileSystem() or HTTPFileSystem() or Snapshot()?
Actions #10

Updated by Tom Clegg about 5 years ago

  • File deleted (index page 0e3369b.png)
Actions #11

Updated by Tom Clegg about 5 years ago

  • File deleted (index page 2dc0e00.png)
Actions #12

Updated by Tom Clegg about 5 years ago

at a42cb73, with file sizes

Actions #14

Updated by Tom Clegg about 5 years ago

8784-dir-listings @ 510a92b885ff547dd7eecb34093f27a7245f021f
  • use new SDK to load into collection record and cache that, instead of loading/caching a map[string]interface and copying to a Collection in each request
Actions #15

Updated by Lucas Di Pentima about 5 years ago

Sorry for the delay, here are some questions:

  • File sdk/go/arvados/collection_fs.go
    • Line 60, 68: Shouldn’t ret be checked if it’s nil before returning? As I read on File’s Readdir doc, if returning an empty slice, it should return a non-nil error.
    • Line 189: I suppose that empty conditional should be removed, right?
  • File services/keep-web/handler.go
    • Line 168: Shouldn't that comment line be updated to something like “/c=ID[/PATH…]” to stay in sync with the code?
    • Lines 323-335: It’s a little difficult (at least for me) to follow that conditionals chain, could you add some comments? I’m specially interested on L329, but in general I feel that for an Open() operation, those are lots of checks, it would be nice to have comments.
    • Line 402: Couldn’t colllection be retrieved from fs doing type assertion? (to avoid passing it as an argument)
  • Don’t understand why, but running local tests failed with this:
    # git.curoverse.com/arvados.git/services/keep-web
    tmp/GOPATH/src/git.curoverse.com/arvados.git/services/keep-web/handler.go:451: undefined: sort.Slice
    
  • If a collection is empty, is it possible to show some legend stating that fact instead of using the title “File Listing”?
Actions #16

Updated by Tom Clegg about 5 years ago

Lucas Di Pentima wrote:

Sorry for the delay, here are some questions:

  • File sdk/go/arvados/collection_fs.go
    • Line 60, 68: Shouldn’t ret be checked if it’s nil before returning? As I read on File’s Readdir doc, if returning an empty slice, it should return a non-nil error.

Yes, updated. If count>0 then an empty slice has to come with an error.

  • Line 189: I suppose that empty conditional should be removed, right?

Whoops. That can't happen: we would have returned it as a file earlier. Removed.

  • File services/keep-web/handler.go
    • Line 168: Shouldn't that comment line be updated to something like “/c=ID[/PATH…]” to stay in sync with the code?

Updated.

  • Lines 323-335: It’s a little difficult (at least for me) to follow that conditionals chain, could you add some comments? I’m specially interested on L329, but in general I feel that for an Open() operation, those are lots of checks, it would be nice to have comments.

Added comments to the error cases.

  • Line 402: Couldn’t colllection be retrieved from fs doing type assertion? (to avoid passing it as an argument)

I suppose... turns out the only thing we need from the collection is its name anyway, so I changed that arg to "collectionName string".

  • Don’t understand why, but running local tests failed with this:
    [...]

Ah, it seems that was introduced in Go 1.8 -- I thought sorting seemed easier than last time! Updated run-tests.sh and packaging scripts accordingly.

  • If a collection is empty, is it possible to show some legend stating that fact instead of using the title “File Listing”?

Done.

8784-dir-listings @ f77d63e6cfaf7278c1cb0fb05e5a4e3f45320e3a

Actions #17

Updated by Lucas Di Pentima about 5 years ago

LGTM!

Actions #18

Updated by Tom Clegg about 5 years ago

8784-dir-listings @ 6c51f11ab5affb4023762227ffb53a5be11a1003
Actions #19

Updated by Tom Clegg about 5 years ago

  • Subject changed from [Keep-web] Generate directory listings to [Workbench] Use keep-web to generate directory listings
  • Story points changed from 2.0 to 0.5
Actions #20

Updated by Tom Clegg about 5 years ago

  • Target version changed from 2017-06-21 sprint to 2017-07-05 sprint
Actions #22

Updated by Peter Amstutz about 5 years ago

What's the rationale for using query_token instead of path_token?

Actions #23

Updated by Tom Clegg about 5 years ago

Peter Amstutz wrote:

What's the rationale for using query_token instead of path_token?

None -- changed to path_token.

Also added a fix to make the "upload / network error" integration test pass with the latest Firefox.

8784-dir-listings @ e59c1d365d9b6e1eff9b5cb030a8b1a3aaf14353

Actions #25

Updated by Tom Clegg about 5 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF