From 41c59f4c47875a10ecfd6d41d49785cdf83a0091 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Wed, 27 Apr 2016 02:17:16 +0200 Subject: [PATCH] Make discovery explicit (#439) * Make discovery explicit Fix #438 --- docs/tutorial.rst | 29 +++++++-------- tests/cli/test_discover.py | 9 ++--- tests/cli/test_sync.py | 73 ++++++++++++++++++++++++-------------- vdirsyncer/cli/__init__.py | 2 +- vdirsyncer/cli/utils.py | 21 ++++++----- 5 files changed, 77 insertions(+), 57 deletions(-) diff --git a/docs/tutorial.rst b/docs/tutorial.rst index 5aea3ab..38bf918 100644 --- a/docs/tutorial.rst +++ b/docs/tutorial.rst @@ -89,12 +89,12 @@ default addressbook to ``~/.contacts/``:: Configuration for other servers can be found at :ref:`supported-servers`. -After running ``vdirsyncer sync``, ``~/.contacts/`` will contain a bunch of -``.vcf`` files which all contain a contact in ``VCARD`` format each. You can -modify their content, add new ones and delete some [1]_, and your changes will be -synchronized to the CalDAV server after you run ``vdirsyncer sync`` again. For -further reference, it uses the storages :storage:`filesystem` and -:storage:`carddav`. +After running ``vdirsyncer discover`` and ``vdirsyncer sync``, ``~/.contacts/`` +will contain a bunch of ``.vcf`` files which all contain a contact in ``VCARD`` +format each. You can modify their content, add new ones and delete some [1]_, +and your changes will be synchronized to the CalDAV server after you run +``vdirsyncer sync`` again. For further reference, it uses the storages +:storage:`filesystem` and :storage:`carddav`. .. [1] You'll want to :doc:`use a helper program for this `. @@ -145,15 +145,16 @@ line to let vdirsyncer automatically sync all addressbooks it can find:: username = bob password = asdf -With the above configuration, vdirsyncer will fetch all available collections -from the server, and create subdirectories for each of them in -``~/.contacts/``. For example, ownCloud's default addressbook ``"default"`` -would be synchronized to the location ``~/.contacts/default/``. +With the above configuration, ``vdirsyncer discover`` will fetch all available +collections from the server, and create subdirectories for each of them in +``~/.contacts/`` after confirmation. For example, ownCloud's default +addressbook ``"default"`` would be synchronized to the location +``~/.contacts/default/``. -Vdirsyncer fetches this list on first sync, and will re-fetch it if you change -your configuration file. However, if new collections are created on the server, -it will not automatically start synchronizing those [2]_. You should run -``vdirsyncer discover`` to re-fetch this list instead. +After that, ``vdirsyncer sync`` will synchronize all your addressbooks as +expected. However, if new collections are created on the server, it will not +automatically start synchronizing those [2]_. You need to run ``vdirsyncer +discover`` again to re-fetch this list instead. .. [2] Because collections are added rarely, and checking for this case before every synchronization isn't worth the overhead. diff --git a/tests/cli/test_discover.py b/tests/cli/test_discover.py index 5213440..0f350c6 100644 --- a/tests/cli/test_discover.py +++ b/tests/cli/test_discover.py @@ -27,18 +27,13 @@ def test_discover_command(tmpdir, runner): bar.mkdir(x) bar.mkdir('d') - result = runner.invoke(['sync']) + result = runner.invoke(['discover']) assert not result.exception - lines = result.output.splitlines() - assert lines[0].startswith('Discovering') - assert 'Syncing foobar/a' in lines - assert 'Syncing foobar/b' in lines - assert 'Syncing foobar/c' in lines - assert 'Syncing foobar/d' not in lines foo.mkdir('d') result = runner.invoke(['sync']) assert not result.exception + lines = result.output.splitlines() assert 'Syncing foobar/a' in lines assert 'Syncing foobar/b' in lines assert 'Syncing foobar/c' in lines diff --git a/tests/cli/test_sync.py b/tests/cli/test_sync.py index c611edc..88883f3 100644 --- a/tests/cli/test_sync.py +++ b/tests/cli/test_sync.py @@ -4,8 +4,6 @@ import json import unicodedata from textwrap import dedent -from click.testing import CliRunner - from hypothesis import example, given import hypothesis.strategies as st @@ -36,6 +34,9 @@ def test_simple_run(tmpdir, runner): tmpdir.mkdir('path_a') tmpdir.mkdir('path_b') + result = runner.invoke(['discover']) + assert not result.exception + result = runner.invoke(['sync']) assert not result.exception @@ -47,6 +48,7 @@ def test_simple_run(tmpdir, runner): def test_sync_inexistant_pair(tmpdir, runner): runner.write_with_general("") + result = runner.invoke(['sync', 'foo']) assert result.exception assert 'pair foo does not exist.' in result.output.lower() @@ -73,6 +75,9 @@ def test_debug_connections(tmpdir, runner): tmpdir.mkdir('path_a') tmpdir.mkdir('path_b') + result = runner.invoke(['discover']) + assert not result.exception + result = runner.invoke(['-vdebug', 'sync', '--max-workers=3']) assert 'using 3 maximal workers' in result.output.lower() @@ -101,6 +106,9 @@ def test_empty_storage(tmpdir, runner): tmpdir.mkdir('path_a') tmpdir.mkdir('path_b') + result = runner.invoke(['discover']) + assert not result.exception + result = runner.invoke(['sync']) assert not result.exception @@ -116,15 +124,9 @@ def test_empty_storage(tmpdir, runner): assert result.exception -def test_verbosity(tmpdir): - runner = CliRunner() - config_file = tmpdir.join('config') - config_file.write('') - - result = runner.invoke( - cli.app, ['--verbosity=HAHA', 'sync'], - env={'VDIRSYNCER_CONFIG': str(config_file)} - ) +def test_verbosity(tmpdir, runner): + runner.write_with_general('') + result = runner.invoke(['--verbosity=HAHA', 'sync']) assert result.exception assert 'invalid value for "--verbosity"' in result.output.lower() @@ -151,6 +153,12 @@ def test_deprecated_item_status(tmpdir): def test_collections_cache_invalidation(tmpdir, runner): + foo = tmpdir.mkdir('foo') + bar = tmpdir.mkdir('bar') + for x in 'abc': + foo.mkdir(x) + bar.mkdir(x) + runner.write_with_general(dedent(''' [storage foo] type = filesystem @@ -168,13 +176,11 @@ def test_collections_cache_invalidation(tmpdir, runner): collections = ["a", "b", "c"] ''').format(str(tmpdir))) - foo = tmpdir.mkdir('foo') - bar = tmpdir.mkdir('bar') - for x in 'abc': - foo.mkdir(x) - bar.mkdir(x) foo.join('a/itemone.txt').write('UID:itemone') + result = runner.invoke(['discover']) + assert not result.exception + result = runner.invoke(['sync']) assert not result.exception assert 'detected change in config file' not in result.output.lower() @@ -208,6 +214,12 @@ def test_collections_cache_invalidation(tmpdir, runner): bar2.mkdir(x) result = runner.invoke(['sync']) assert 'detected change in config file' in result.output.lower() + assert result.exception + + result = runner.invoke(['discover']) + assert not result.exception + + result = runner.invoke(['sync']) assert not result.exception rv = bar.join('a').listdir() @@ -239,6 +251,9 @@ def test_invalid_pairs_as_cli_arg(tmpdir, runner): for c in 'abc': base.mkdir(c) + result = runner.invoke(['discover']) + assert not result.exception + result = runner.invoke(['sync', 'foobar/d']) assert result.exception assert 'pair foobar: collection d not found' in result.output.lower() @@ -258,19 +273,25 @@ def test_multiple_pairs(tmpdir, runner): yield dedent(''' [storage {name}] type = filesystem - path = {base}/{name}/ + path = {path} fileext = .txt - ''').format(name=name, base=str(tmpdir)) + ''').format(name=name, path=str(tmpdir.mkdir(name))) runner.write_with_general(''.join(get_cfg())) - result = runner.invoke(['sync']) + result = runner.invoke(['discover']) assert set(result.output.splitlines()) > set([ 'Discovering collections for pair bambaz', - 'Discovering collections for pair foobar', + 'Discovering collections for pair foobar' + ]) + assert not result.exception + + result = runner.invoke(['sync']) + assert set(result.output.splitlines()) == set([ 'Syncing bambaz', 'Syncing foobar', ]) + assert not result.exception @given(collections=st.sets( @@ -311,13 +332,8 @@ def test_create_collections(subtest, collections): fileext = .txt '''.format(base=str(tmpdir), colls=json.dumps(list(collections))))) - result = runner.invoke(['sync']) - assert result.exception - entries = set(x.basename for x in tmpdir.listdir()) - assert 'foo' not in entries and 'bar' not in entries - result = runner.invoke( - ['sync'], + ['discover'], input='y\n' * 2 * (len(collections) + 1) ) assert not result.exception @@ -369,6 +385,9 @@ def test_ident_conflict(tmpdir, runner): foo.join('two.txt').write('UID:1') foo.join('three.txt').write('UID:1') + result = runner.invoke(['discover']) + assert not result.exception + result = runner.invoke(['sync']) assert result.exception assert ('error: foobar: Storage "foo" contains multiple items with the ' @@ -399,7 +418,7 @@ def test_unknown_storage(tmpdir, runner, existing, missing): tmpdir.mkdir(existing) - result = runner.invoke(['sync']) + result = runner.invoke(['discover']) assert result.exception assert ( diff --git a/vdirsyncer/cli/__init__.py b/vdirsyncer/cli/__init__.py index ae3b0ec..846c895 100644 --- a/vdirsyncer/cli/__init__.py +++ b/vdirsyncer/cli/__init__.py @@ -208,7 +208,7 @@ def discover(ctx, pairs, max_workers, list): discover_collections, status_path=config.general['status_path'], pair=pair, - skip_cache=True, + from_cache=False, list_collections=list, )) wq.spawn_worker() diff --git a/vdirsyncer/cli/utils.py b/vdirsyncer/cli/utils.py index 130e24f..81f352f 100644 --- a/vdirsyncer/cli/utils.py +++ b/vdirsyncer/cli/utils.py @@ -156,27 +156,32 @@ def _get_collections_cache_key(pair): return m.hexdigest() -def collections_for_pair(status_path, pair, skip_cache=False, +def collections_for_pair(status_path, pair, from_cache=True, list_collections=False): '''Determine all configured collections for a given pair. Takes care of shortcut expansion and result caching. :param status_path: The path to the status directory. - :param skip_cache: Whether to skip the cached data and always do discovery. - Even with this option enabled, the new cache is written. + :param from_cache: Whether to load from cache (aborting on cache miss) or + discover and save to cache. :returns: iterable of (collection, (a_args, b_args)) ''' - rv = load_status(status_path, pair.name, data_type='collections') cache_key = _get_collections_cache_key(pair) - if rv and not skip_cache: - if rv.get('cache_key', None) == cache_key: + if from_cache: + rv = load_status(status_path, pair.name, data_type='collections') + if rv and rv.get('cache_key', None) == cache_key: return list(_expand_collections_cache( rv['collections'], pair.config_a, pair.config_b )) elif rv: - cli_logger.info('Detected change in config file, discovering ' - 'collections for {}'.format(pair.name)) + raise exceptions.UserError('Detected change in config file, ' + 'please run `vdirsyncer discover {}`.' + .format(pair.name)) + else: + raise exceptions.UserError('Please run `vdirsyncer discover {}` ' + ' before synchronization.' + .format(pair.name)) cli_logger.info('Discovering collections for pair {}' .format(pair.name))