Story #8784

[Workbench] Use keep-web to generate directory listings

Added by Tom Clegg over 1 year ago. Updated 5 months ago.

Status:ResolvedStart date:03/23/2016
Priority:NormalDue date:
Assignee:Tom Clegg% Done:

100%

Category:Keep
Target version:2017-07-05 sprint
Story points0.5Remaining (hours)0.00 hour
Velocity based estimate0 days

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

index.page.2dc0e00.png (66.6 KB) Tom Clegg, 06/13/2017 01:00 pm

index.page.0e3369b.png (79.8 KB) Tom Clegg, 06/13/2017 01:00 pm

index.page.a42cb73.png (67.3 KB) Tom Clegg, 06/13/2017 02:00 pm


Subtasks

Task #11887: Review 8784-dir-listingsResolvedPeter Amstutz

Task #11872: Workbench redirectResolvedTom Clegg

Task #11834: Review 8784-dir-listingsResolvedTom Clegg


Related issues

Related to Arvados - Story #5824: [Workbench] [Keep] collection browse/download server Resolved 05/21/2015
Related to Arvados - Story #11167: [Workbench] Remove arv-get file download fallback Resolved 07/24/2017

Associated revisions

Revision 04efddf6
Added by Tom Clegg 5 months ago

Merge branch '8784-dir-listings'

refs #8784

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision a249cd98
Added by Tom Clegg 5 months ago

8784: Update arvbox to go1.8.

refs #8784

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision 0eb72b52
Added by Tom Clegg 5 months ago

Merge branch '8784-dir-listings'

refs #8784

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Brett Smith over 1 year ago

  • Target version set to Arvados Future Sprints

#2 Updated by Peter Amstutz 6 months ago

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

#3 Updated by Tom Morris 6 months ago

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

#4 Updated by Tom Morris 6 months ago

  • Description updated (diff)
  • Story points set to 2.0

#5 Updated by Tom Morris 6 months ago

  • Tracker changed from Bug to Story

#6 Updated by Tom Clegg 6 months ago

  • Assignee set to Tom Clegg

#7 Updated by Tom Clegg 6 months ago

  • Status changed from New to In Progress

#8 Updated by Tom Clegg 5 months ago

  • File index page 0e3369b.png added
  • File index page 2dc0e00.png added

#9 Updated by Tom Clegg 5 months 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()?

#10 Updated by Tom Clegg 5 months ago

  • File deleted (index page 0e3369b.png)

#11 Updated by Tom Clegg 5 months ago

  • File deleted (index page 2dc0e00.png)

#12 Updated by Tom Clegg 5 months ago

at a42cb73, with file sizes

#13 Updated by Tom Clegg 5 months ago

#14 Updated by Tom Clegg 5 months 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

#15 Updated by Lucas Di Pentima 5 months 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”?

#16 Updated by Tom Clegg 5 months 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

#17 Updated by Lucas Di Pentima 5 months ago

LGTM!

#18 Updated by Tom Clegg 5 months ago

8784-dir-listings @ 6c51f11ab5affb4023762227ffb53a5be11a1003

#19 Updated by Tom Clegg 5 months 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

#20 Updated by Tom Clegg 5 months ago

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

#22 Updated by Peter Amstutz 5 months ago

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

#23 Updated by Tom Clegg 5 months 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

#25 Updated by Tom Clegg 5 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF