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 " \