Story #2035

arv-mount exposes filesystem paths like /tag/tagname/collection-uuid and /folder/foldername/collection-uuid

Added by Tom Clegg over 7 years ago. Updated over 7 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
04/29/2014
Due date:
% Done:

100%

Estimated time:
(Total: 25.00 h)
Story points:
3.0
Release relationship:
Auto

Subtasks

Task #2709: Review 2035-arv-mount-tags-folders at revision b4a9aaaResolvedPeter Amstutz

Task #2742: Review 2035-arv-mount-tags-foldersResolvedPeter Amstutz

Task #2607: arv-mount exposes tags at {mountpoint}/tag/{tag_text}ResolvedPeter Amstutz

Task #2623: Add 'select' and 'distinct' to API server, required to get request a list of distinct tagsResolvedPeter Amstutz

Task #2606: arv-mount exposes different object types at {mountpoint}/{objecttype}/{uuid}ResolvedPeter Amstutz

History

#1 Updated by Tom Clegg over 7 years ago

  • Target version set to 2014-03-05 Data management

#2 Updated by Tom Clegg over 7 years ago

  • Story points set to 2.0

#3 Updated by Tom Clegg over 7 years ago

  • Subject changed from Keep's POSIX interface can give access to collections via names and tags, not just via manifest hashes. to Keep's POSIX interface can give access to collections via names, not just via manifest hashes.

#4 Updated by Tom Clegg over 7 years ago

  • Target version changed from 2014-03-05 Data management to 2014-05-07 Storing and Organizing Data

#5 Updated by Tom Clegg over 7 years ago

  • Subject changed from Keep's POSIX interface can give access to collections via names, not just via manifest hashes. to Keep's POSIX interface can give access to collections via projects, not just manifest hashes.

#6 Updated by Tom Clegg over 7 years ago

  • Subject changed from Keep's POSIX interface can give access to collections via projects, not just manifest hashes. to arv-mount exposes filesystem paths like /tag/tagname/collection-uuid and /folder/foldername/collection-uuid

#7 Updated by Peter Amstutz over 7 years ago

  • Assigned To set to Peter Amstutz

#8 Updated by Peter Amstutz over 7 years ago

  • Story points changed from 2.0 to 3.0

#9 Updated by Tom Clegg over 7 years ago

Looking at b4a9aaa

This looks like it won't play well with "attr asc" and "attr desc" forms (and seems not to have any tests): update, fixed in de54cdc

@orders.select! { |o| @select.include? o } if @select

Add a test with more "select" than "distinct" columns (or no "select" at all), and verify that the "distinct" behavior actually occurs

Mention at doc/api/methods.html

Otherwise, LGTM

#10 Updated by Tom Clegg over 7 years ago

In docs: prefer "false (default)" rather than "default; false:" -- and/or mention the false case first. Currently it's easy to read as if true is the default.

#11 Updated by Brett Smith over 7 years ago

Reviewing e6aa5b4. I love a well-tested branch. Thanks for building all this necessary infrastructure; I'm sure I'll be building on top of it before I know it. My comments are hardly about the code itself; more about glue.

The one thing I'd say about the updated driver is that there seems to be a lot of similar code, especially in the event bus subscriptions and invalidate methods. The merge method does a great job of abstracting away common functionality—I wonder if it'd be possible to use a similar strategy to refactor these too? But this is more along the lines of style and I don't think it needs to hold up the merge before the end of the sprint.

The setup.py templates assume that we launch it from its own directory when they rmtree. Because this is potentially destructive, I think it's worth guarding to make sure we work on the correct build directory. It also causes the script to fail if we haven't built anything yet, or didn't last time. Maybe something like:

import os
import sys
build_dir = os.path.join(os.path.dirname(sys.argv[0]), 'build')
shutil.rmtree(build_dir, ignore_errors=True)

And then a couple small things in run_test_server.py:find_server_pid:

  • Testing server_pid > 0 is extraneous. It's in the Ruby because ''.to_i == 0 (i.e., the pidfile is empty), but in Python int('') is a ValueError. I would expect the outcomes here to always be IOError, OSError, ValueError, or a good pid.
  • Naming those exceptions, or at least catching Exception, would be helpful to avoid catching low-level exceptions like KeyboardInterrupt (since it's conceivable the user might try to ^C during the loop sleep).

Thanks.

#12 Updated by Brett Smith over 7 years ago

One small additional note: setup_fuse.py should be added to the Python SDK .gitignore alongside setup.py.

#13 Updated by Peter Amstutz over 7 years ago

The event bus and invalidation code is more or less just the minimum required to ensure that API server updates are visible to the user. A better implementation would actually look at the event to determine what to do with it.

I applied your suggestion for selecting the correct directory, and that inspired me to refactor the setup scripts to eliminate build.sh and sed.

I applied the changes you suggested to run_test_server.py

The .gitignore comment is no longer applicable because setup.py and setup_fuse.py are no longer generated from setup.py.src and setup_fuse.py.src.

#14 Updated by Brett Smith over 7 years ago

Peter Amstutz wrote:

I applied your suggestion for selecting the correct directory, and that inspired me to refactor the setup scripts to eliminate build.sh and sed.

That's even way better, thank you. This version looks good to merge to me.

#15 Updated by Peter Amstutz over 7 years ago

  • Status changed from New to Resolved

Also available in: Atom PDF