From 3b419c3bc41c6a3db84d4456e0d757709831e2ab Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Fri, 19 Oct 2018 18:39:11 +0100 Subject: [PATCH] docs: Add docstring information for pegleg.config Pegleg's config module is used for basically keeping track of CLI context information within global memory namespace. None of the functions therein are properly documented which can lead to confusion. This patch set adds clarifying documentation for all the functions. Change-Id: I93545331ffc3a83b593f654ee90fb6af3d067402 --- pegleg/cli.py | 15 ++++--- pegleg/config.py | 48 +++++++++++++++++++---- pegleg/engine/repository.py | 6 +-- pegleg/engine/util/git.py | 18 ++++++--- tests/unit/conftest.py | 1 + tests/unit/engine/test_site_repository.py | 12 +++--- tests/unit/test_cli.py | 40 +++++++++---------- 7 files changed, 88 insertions(+), 52 deletions(-) diff --git a/pegleg/cli.py b/pegleg/cli.py index cfcc574e..a605b3e5 100644 --- a/pegleg/cli.py +++ b/pegleg/cli.py @@ -69,8 +69,7 @@ REPOSITORY_CLONE_PATH_OPTION = click.option( '-p', '--clone-path', 'clone_path', - help= - 'The path where the repo will be cloned. By default the repo will be ' + help='The path where the repo will be cloned. By default the repo will be ' 'cloned to the /tmp path. If this option is included and the repo already ' 'exists, then the repo will not be cloned again and the user must specify ' 'a new clone path or pass in the local copy of the repository as the site ' @@ -188,8 +187,8 @@ def lint_repo(*, fail_on_missing_sub_src, exclude_lint, warn_lint): @EXTRA_REPOSITORY_OPTION @REPOSITORY_USERNAME_OPTION @REPOSITORY_KEY_OPTION -def site(*, site_repository, clone_path, extra_repositories, - repo_key, repo_username): +def site(*, site_repository, clone_path, extra_repositories, repo_key, + repo_username): """Group for site-level actions, which include: * list: list available sites in a manifests repo @@ -201,7 +200,7 @@ def site(*, site_repository, clone_path, extra_repositories, config.set_site_repo(site_repository) config.set_clone_path(clone_path) - config.set_extra_repo_store(extra_repositories or []) + config.set_extra_repo_overrides(extra_repositories or []) config.set_repo_key(repo_key) config.set_repo_username(repo_username) @@ -324,8 +323,8 @@ def lint_site(*, fail_on_missing_sub_src, exclude_lint, warn_lint, site_name): @EXTRA_REPOSITORY_OPTION @REPOSITORY_USERNAME_OPTION @REPOSITORY_KEY_OPTION -def type(*, site_repository, clone_path, extra_repositories, - repo_key, repo_username): +def type(*, site_repository, clone_path, extra_repositories, repo_key, + repo_username): """Group for repo-level actions, which include: * list: list all types across the repository @@ -333,7 +332,7 @@ def type(*, site_repository, clone_path, extra_repositories, """ config.set_site_repo(site_repository) config.set_clone_path(clone_path) - config.set_extra_repo_store(extra_repositories or []) + config.set_extra_repo_overrides(extra_repositories or []) config.set_repo_key(repo_key) config.set_repo_username(repo_username) diff --git a/pegleg/config.py b/pegleg/config.py index ffc09c1c..8cf0a616 100644 --- a/pegleg/config.py +++ b/pegleg/config.py @@ -12,6 +12,10 @@ # See the License for the specific language governing permissions and # limitations under the License. +# TODO(felipemonteiro): This pattern below should be swapped out for click +# context passing but will require a somewhat heavy code refactor. See: +# http://click.pocoo.org/5/commands/#nested-handling-and-contexts + try: if GLOBAL_CONTEXT: pass @@ -26,81 +30,109 @@ except NameError: def get_site_repo(): + """Get the primary site repository specified via ``-r`` CLI flag.""" return GLOBAL_CONTEXT['site_repo'] def set_site_repo(r): + """Set the primary site repository.""" GLOBAL_CONTEXT['site_repo'] = r.rstrip('/') + '/' def get_clone_path(): + """Get specified clone path (corresponds to ``-p`` CLI flag).""" return GLOBAL_CONTEXT['clone_path'] def set_clone_path(p): + """Set specified clone path (corresponds to ``-p`` CLI flag).""" GLOBAL_CONTEXT['clone_path'] = p -def get_extra_repo_store(): - return GLOBAL_CONTEXT.get('extra_repo_store', []) +def get_extra_repo_overrides(): + """Get extra repository overrides specified via ``-e`` CLI flag.""" + return GLOBAL_CONTEXT.get('extra_repo_overrides', []) -def set_extra_repo_store(r): - GLOBAL_CONTEXT['extra_repo_store'] = r +def set_extra_repo_overrides(r): + """Set extra repository overrides. + + .. note:: Only CLI should call this. + """ + GLOBAL_CONTEXT['extra_repo_overrides'] = r def set_repo_key(k): + """Set additional repository key, like extra metadata to track.""" GLOBAL_CONTEXT['repo_key'] = k def get_repo_key(): + """Get additional repository key.""" return GLOBAL_CONTEXT.get('repo_key', None) def set_repo_username(u): + """Set repo username for SSH auth, corresponds to ``-u`` CLI flag.""" GLOBAL_CONTEXT['repo_username'] = u def get_repo_username(): + """Get repo username for SSH auth.""" return GLOBAL_CONTEXT.get('repo_username', None) def set_extra_repo_list(a): + """Set the extra repository list to be used by ``pegleg.engine``.""" GLOBAL_CONTEXT['extra_repos'] = [r.rstrip('/') + '/' for r in a] -def add_extra_repo(a): - GLOBAL_CONTEXT['extra_repos'].append(a.rstrip('/') + '/') - - def get_extra_repo_list(): + """Get the extra repository list. + + .. note:: + + Use this instead of ``get_extra_repo_overrides`` as it handles + both overrides and site-definition.yaml defaults. + """ return GLOBAL_CONTEXT['extra_repos'] +def add_extra_repo(a): + """Add an extra repo to the extra repository list.""" + GLOBAL_CONTEXT['extra_repos'].append(a.rstrip('/') + '/') + + def each_extra_repo(): + """Iterate over each extra repo.""" for a in GLOBAL_CONTEXT['extra_repos']: yield a def all_repos(): + """Return the primary site repo, in addition to all extra ones.""" repos = [get_site_repo()] repos.extend(get_extra_repo_list()) return repos def get_rel_site_path(): + """Get the relative site path name, default is "site".""" return GLOBAL_CONTEXT.get('site_path', 'site') def set_rel_site_path(p): + """Set the relative site path name.""" p = p or 'site' GLOBAL_CONTEXT['site_path'] = p def get_rel_type_path(): + """Get the relative type path name, default is "type".""" return GLOBAL_CONTEXT.get('type_path', 'type') def set_rel_type_path(p): + """Set the relative type path name.""" p = p or 'type' GLOBAL_CONTEXT['type_path'] = p diff --git a/pegleg/engine/repository.py b/pegleg/engine/repository.py index fcea6047..750e1362 100644 --- a/pegleg/engine/repository.py +++ b/pegleg/engine/repository.py @@ -234,7 +234,7 @@ def _process_repository_overrides(site_def_repos): """ # Extra repositories to process. - provided_repo_overrides = config.get_extra_repo_store() + provided_repo_overrides = config.get_extra_repo_overrides() # Map repository names to the associated URL/revision for cloning. repo_overrides = {} @@ -312,8 +312,8 @@ def _handle_repository(repo_url_or_path, *args, **kwargs): clone_path = config.get_clone_path() try: - return util.git.git_handler(repo_url_or_path, clone_path=clone_path, - *args, **kwargs) + return util.git.git_handler( + repo_url_or_path, clone_path=clone_path, *args, **kwargs) except exceptions.GitException as e: raise click.ClickException(e) except Exception as e: diff --git a/pegleg/engine/util/git.py b/pegleg/engine/util/git.py index e638d7c7..7c92e0a6 100644 --- a/pegleg/engine/util/git.py +++ b/pegleg/engine/util/git.py @@ -29,8 +29,11 @@ LOG = logging.getLogger(__name__) __all__ = ('git_handler', ) -def git_handler(repo_url, ref=None, proxy_server=None, - auth_key=None, clone_path=None): +def git_handler(repo_url, + ref=None, + proxy_server=None, + auth_key=None, + clone_path=None): """Handle directories that are Git repositories. If ``repo_url`` is a valid URL for which a local repository doesn't @@ -75,8 +78,8 @@ def git_handler(repo_url, ref=None, proxy_server=None, # we need to clone the repo_url first since it doesn't exist and then # checkout the appropriate reference - and return the tmpdir if parsed_url.scheme in supported_clone_protocols: - return _try_git_clone(repo_url, ref, proxy_server, - auth_key, clone_path) + return _try_git_clone(repo_url, ref, proxy_server, auth_key, + clone_path) else: raise ValueError('repo_url=%s must use one of the following ' 'protocols: %s' % @@ -142,8 +145,11 @@ def _get_current_ref(repo_url): return None -def _try_git_clone(repo_url, ref=None, proxy_server=None, - auth_key=None, clone_path=None): +def _try_git_clone(repo_url, + ref=None, + proxy_server=None, + auth_key=None, + clone_path=None): """Try cloning Git repo from ``repo_url`` using the reference ``ref``. :param repo_url: URL of remote Git repo or path to local Git repo. diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index a05f82b4..fc0b171e 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -50,6 +50,7 @@ def clean_temporary_git_repos(): if os.path.isdir(path) and os.access(path, os.R_OK): if any(p.startswith('airship') for p in os.listdir(path)): yield path + try: yield finally: diff --git a/tests/unit/engine/test_site_repository.py b/tests/unit/engine/test_site_repository.py index ebfe3c6c..b7a20523 100644 --- a/tests/unit/engine/test_site_repository.py +++ b/tests/unit/engine/test_site_repository.py @@ -183,7 +183,7 @@ def _test_process_repositories(site_repo=None, elif repo_overrides: with mock.patch.object( config, - 'get_extra_repo_store', + 'get_extra_repo_overrides', autospec=True, return_value=list(repo_overrides.values())): do_test() @@ -301,7 +301,7 @@ def test_process_repositiories_extraneous_user_repo_value(m_log, *_): # Get rid of REPO_USERNAME through an override. with mock.patch.object( config, - 'get_extra_repo_store', + 'get_extra_repo_overrides', autospec=True, return_value=repo_overrides): _test_process_repositories_inner( @@ -355,7 +355,7 @@ def test_process_repositiories_no_site_def_repos_with_extraneous_overrides( # Provide repo overrides. with mock.patch.object( config, - 'get_extra_repo_store', + 'get_extra_repo_overrides', autospec=True, return_value=repo_overrides): _test_process_repositories_inner( @@ -395,12 +395,12 @@ def test_process_repositories_without_repositories_key_in_site_definition( autospec=True, return_value=TEST_REPOSITORIES) @mock.patch.object(util.git, 'is_repository', autospec=True, return_value=True) -@mock.patch.object(config, 'get_extra_repo_store', autospec=True) +@mock.patch.object(config, 'get_extra_repo_overrides', autospec=True) def test_process_extra_repositories_malformed_format_raises_exception( - m_get_extra_repo_store, *_): + m_get_extra_repo_overrides, *_): # Will fail since it doesn't contain "=". broken_repo_url = 'broken_url' - m_get_extra_repo_store.return_value = [broken_repo_url] + m_get_extra_repo_overrides.return_value = [broken_repo_url] error = ("The repository %s must be in the form of " "name=repoUrl[@revision]" % broken_repo_url) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 2bc37fc6..ad4c34c8 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -26,6 +26,7 @@ from pegleg.engine.util import git from tests.unit import test_utils from tests.unit.fixtures import temp_clone_path + @pytest.mark.skipif( not test_utils.is_connected(), reason='git clone requires network connectivity.') @@ -59,8 +60,7 @@ class TestSiteCLIOptions(BaseCLIActionTest): ### clone_path tests ### def test_list_sites_using_remote_repo_and_clone_path_option( - self, - temp_clone_path): + self, temp_clone_path): """Validates clone_path (-p) option is working properly with site list action when using remote repo. Verify that the repo was cloned in the clone_path @@ -74,18 +74,16 @@ class TestSiteCLIOptions(BaseCLIActionTest): self.repo_rev) # Note that the -p option is used to specify the clone_folder - site_list = self.runner.invoke(cli.site, ['-p', temp_clone_path, - '-r', repo_url, 'list']) + site_list = self.runner.invoke( + cli.site, ['-p', temp_clone_path, '-r', repo_url, 'list']) assert site_list.exit_code == 0 # Verify that the repo was cloned into the clone_path - assert os.path.exists(os.path.join(temp_clone_path, - self.repo_name)) - assert git.is_repository(os.path.join(temp_clone_path, - self.repo_name)) + assert os.path.exists(os.path.join(temp_clone_path, self.repo_name)) + assert git.is_repository(os.path.join(temp_clone_path, self.repo_name)) - def test_list_sites_using_local_repo_and_clone_path_option(self, - temp_clone_path): + def test_list_sites_using_local_repo_and_clone_path_option( + self, temp_clone_path): """Validates clone_path (-p) option is working properly with site list action when using a local repo. Verify that the clone_path has NO effect when using a local repo @@ -98,12 +96,13 @@ class TestSiteCLIOptions(BaseCLIActionTest): repo_path = self.treasuremap_path # Note that the -p option is used to specify the clone_folder - site_list = self.runner.invoke(cli.site, ['-p', temp_clone_path, - '-r', repo_path, 'list']) + site_list = self.runner.invoke( + cli.site, ['-p', temp_clone_path, '-r', repo_path, 'list']) assert site_list.exit_code == 0 # Verify that passing in clone_path when using local repo has no effect - assert not os.path.exists(os.path.join(temp_clone_path, self.repo_name)) + assert not os.path.exists( + os.path.join(temp_clone_path, self.repo_name)) class TestSiteCLIOptionsNegative(BaseCLIActionTest): @@ -111,8 +110,8 @@ class TestSiteCLIOptionsNegative(BaseCLIActionTest): ### Negative clone_path tests ### - def test_list_sites_using_remote_repo_and_reuse_clone_path_option(self, - temp_clone_path): + def test_list_sites_using_remote_repo_and_reuse_clone_path_option( + self, temp_clone_path): """Validates clone_path (-p) option is working properly with site list action when using remote repo. Verify that the same repo can't be cloned in the same clone_path if it already exists @@ -126,16 +125,15 @@ class TestSiteCLIOptionsNegative(BaseCLIActionTest): self.repo_rev) # Note that the -p option is used to specify the clone_folder - site_list = self.runner.invoke(cli.site, ['-p', temp_clone_path, - '-r', repo_url, 'list']) + site_list = self.runner.invoke( + cli.site, ['-p', temp_clone_path, '-r', repo_url, 'list']) - assert git.is_repository(os.path.join(temp_clone_path, - self.repo_name)) + assert git.is_repository(os.path.join(temp_clone_path, self.repo_name)) # Run site list for a second time to validate that the repo can't be # cloned twice in the same clone_path - site_list = self.runner.invoke(cli.site, ['-p', temp_clone_path, - '-r', repo_url, 'list']) + site_list = self.runner.invoke( + cli.site, ['-p', temp_clone_path, '-r', repo_url, 'list']) assert site_list.exit_code == 1 msg = "The repository already exists in the given path. Either " \