From b8733ea7ec4555aab584c0b9af2b908942b9f5a9 Mon Sep 17 00:00:00 2001 From: Lev Morgan Date: Mon, 1 Apr 2019 15:01:52 -0500 Subject: [PATCH] Make -u required in CLI when required by repo This patch detects when a repository URL requires username substitution and raises an exception when no username was specified. Change-Id: Ia60982ecddd957cff8709118b3eb8a905258dd06 --- doc/source/cli/cli.rst | 4 +- pegleg/cli.py | 3 +- pegleg/engine/exceptions.py | 5 ++ pegleg/engine/repository.py | 4 ++ tests/unit/engine/test_site_repository.py | 69 +++++++++++++++++------ 5 files changed, 66 insertions(+), 19 deletions(-) diff --git a/doc/source/cli/cli.rst b/doc/source/cli/cli.rst index 4eb5c02b..0686e835 100644 --- a/doc/source/cli/cli.rst +++ b/doc/source/cli/cli.rst @@ -195,7 +195,7 @@ The SSH public key to use when cloning remote authenticated repositories. Required for cloning repositories via SSH protocol. -**-u / --repo-username** (Optional, SSH only). +**-u / --repo-username** (Optional, unless required by repo URL). The SSH username to use when cloning remote authenticated repositories specified in the site-definition file. Any occurrences of ``REPO_USERNAME`` @@ -203,6 +203,8 @@ in an entry under the ``repositories`` field in a given :file:`site-definition.yaml` will be replaced with this value. Required for cloning repositories via SSH protocol. +This argument will generate an exception if no repo URL +uses ``REPO_USERNAME``. Examples ^^^^^^^^ diff --git a/pegleg/cli.py b/pegleg/cli.py index 23b8ebf9..a83b81d4 100644 --- a/pegleg/cli.py +++ b/pegleg/cli.py @@ -81,7 +81,8 @@ REPOSITORY_USERNAME_OPTION = click.option( help='The SSH username to use when cloning remote authenticated ' 'repositories specified in the site-definition file. Any ' 'occurrences of REPO_USERNAME will be replaced with this ' - 'value.') + 'value.\n' + 'Use only if REPO_USERNAME appears in a repo URL.') REPOSITORY_CLONE_PATH_OPTION = click.option( '-p', diff --git a/pegleg/engine/exceptions.py b/pegleg/engine/exceptions.py index 6be2622b..f1d00ec6 100644 --- a/pegleg/engine/exceptions.py +++ b/pegleg/engine/exceptions.py @@ -67,6 +67,11 @@ class GitInvalidRepoException(PeglegBaseException): message = 'The repository path or URL is invalid: %(repo_path)s' +class GitMissingUserException(PeglegBaseException): + """Exception raised when a username is required, but not provided.""" + message = 'Repo URL %(url)s reuqires a username, but none was provided.' + + # # PKI EXCEPTIONS # diff --git a/pegleg/engine/repository.py b/pegleg/engine/repository.py index b7ea0635..033861cd 100644 --- a/pegleg/engine/repository.py +++ b/pegleg/engine/repository.py @@ -327,6 +327,10 @@ def _format_url_with_repo_username(repo_url_or_path): else: repo_url_or_path = repo_url_or_path.replace( 'REPO_USERNAME', repo_user) + + elif "REPO_USERNAME" in repo_url_or_path: + raise exceptions.GitMissingUserException(url=repo_url_or_path) + return repo_url_or_path diff --git a/tests/unit/engine/test_site_repository.py b/tests/unit/engine/test_site_repository.py index 148df8e1..cc01fc73 100644 --- a/tests/unit/engine/test_site_repository.py +++ b/tests/unit/engine/test_site_repository.py @@ -12,10 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -import copy import mock import os -import yaml import click import pytest @@ -25,6 +23,8 @@ from pegleg.engine import exceptions from pegleg.engine import repository from pegleg.engine import util +REPO_USERNAME="test_username" + TEST_REPOSITORIES = { 'repositories': { 'global': { @@ -40,6 +40,23 @@ TEST_REPOSITORIES = { } } +FORMATTED_REPOSITORIES = { + 'repositories': { + 'global': { + 'revision': '843d1a50106e1f17f3f722e2ef1634ae442fe68f', + 'url': 'ssh://{}@gerrit:29418/aic-clcp-manifests.git'.format( + REPO_USERNAME) + }, + 'secrets': { + 'revision': + 'master', + 'url': ('ssh://{}@gerrit:29418/aic-clcp-security-' + 'manifests.git'.format(REPO_USERNAME)) + } + } +} + +config.set_repo_username(REPO_USERNAME) @pytest.fixture(autouse=True) def clean_temp_folders(): @@ -129,7 +146,7 @@ def _test_process_repositories(site_repo=None, ] mock_calls.extend([ mock.call(r['url'], ref=r['revision'], auth_key=None) - for r in TEST_REPOSITORIES['repositories'].values() + for r in FORMATTED_REPOSITORIES['repositories'].values() ]) m_clone_repo.assert_has_calls(mock_calls) elif repo_username: @@ -140,7 +157,7 @@ def _test_process_repositories(site_repo=None, r['url'].replace('REPO_USERNAME', repo_username), ref=r['revision'], auth_key=None) - for r in TEST_REPOSITORIES['repositories'].values() + for r in FORMATTED_REPOSITORIES['repositories'].values() ]) elif repo_overrides: # This is computed from: len(cloned extra repos) + @@ -148,7 +165,7 @@ def _test_process_repositories(site_repo=None, expected_call_count = len(TEST_REPOSITORIES['repositories']) + 1 assert (expected_call_count == m_clone_repo.call_count) - for x, r in TEST_REPOSITORIES['repositories'].items(): + for x, r in FORMATTED_REPOSITORIES['repositories'].items(): if x in expected_repo_overrides: repo_url = expected_repo_overrides[x]['url'] ref = expected_repo_overrides[x]['revision'] @@ -161,7 +178,7 @@ def _test_process_repositories(site_repo=None, else: m_clone_repo.assert_has_calls([ mock.call(r['url'], ref=r['revision'], auth_key=None) - for r in TEST_REPOSITORIES['repositories'].values() + for r in FORMATTED_REPOSITORIES['repositories'].values() ]) if site_repo: @@ -195,19 +212,13 @@ def test_process_repositories(): def test_process_repositories_with_site_repo_url(): """Test process_repository when site repo is a remote URL.""" - # With REPO_USERNAME. - site_repo = ( - 'ssh://REPO_USERNAME@gerrit:29418/aic-clcp-site-manifests.git@333') - _test_process_repositories( - site_repo=site_repo, - expected_repo_url=( - "ssh://REPO_USERNAME@gerrit:29418/aic-clcp-site-manifests"), - expected_repo_revision="333") # Without REPO_USERNAME. - site_repo = 'ssh://gerrit:29418/aic-clcp-site-manifests.git@333' + site_repo = 'ssh://{}:29418/aic-clcp-site-manifests.git@333'.format( + REPO_USERNAME) _test_process_repositories( site_repo=site_repo, - expected_repo_url="ssh://gerrit:29418/aic-clcp-site-manifests", + expected_repo_url="ssh://{}:29418/aic-clcp-site-manifests".format( + REPO_USERNAME), expected_repo_revision="333") @@ -257,7 +268,8 @@ def test_process_repositories_with_repo_overrides_remote_urls(): } expected_repo_overrides = { 'global': { - 'url': 'ssh://REPO_USERNAME@gerrit:29418/aic-clcp-manifests', + 'url': 'ssh://{}@gerrit:29418/aic-clcp-manifests'.format( + REPO_USERNAME), 'revision': '12345' }, } @@ -515,3 +527,26 @@ def test_process_site_repository(_): _do_test( 'ssh://foo@opendev.org/airship/treasuremap:12345', expected='ssh://foo@opendev.org/airship/treasuremap:12345') + + +def test_format_url_with_repo_username(): + TEST_URL = 'ssh://REPO_USERNAME@gerrit:29418/airship/pegleg' + + with mock.patch.object( + config, + 'get_repo_username', + autospec=True, + return_value=REPO_USERNAME): + res = repository._format_url_with_repo_username(TEST_URL) + assert res == 'ssh://{}@gerrit:29418/airship/pegleg'.format( + REPO_USERNAME) + + with mock.patch.object( + config, + 'get_repo_username', + autospec=True, + return_value=''): + pytest.raises( + exceptions.GitMissingUserException, + repository._format_url_with_repo_username, + TEST_URL)