From 11c5888f1719c5309f079b8bb3063e1d49ffbb15 Mon Sep 17 00:00:00 2001 From: Justintime50 <39606064+Justintime50@users.noreply.github.com> Date: Tue, 2 Nov 2021 21:52:22 -0600 Subject: [PATCH] fix: bug fixes and improvements (closes #40) --- CHANGELOG.md | 6 ++++++ github_archive/archive.py | 21 +++++++++++++-------- setup.py | 2 +- test/unit/test_archive.py | 35 +++++++++++++++++++++++++++++++++-- 4 files changed, 53 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec6a14c..92524e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # CHANGELOG +## v4.1.1 (2021-11-02) + +* Fixes a bug that wouldn't allow for gist cloning/pulling because of a bad "forks" check on a gist GitHub object +* Adds a missing check to ensure that at least one CLI argument was passed (closes #40) +* No longer invoke a shell while using the subprocess module. Git operations should now be more stable across operating systems + ## v4.1.0 (2021-09-19) * Adds a new `--https` flag which will authenticate via HTTPS instead of the default `SSH` diff --git a/github_archive/archive.py b/github_archive/archive.py index 4ba242a..4849a6e 100644 --- a/github_archive/archive.py +++ b/github_archive/archive.py @@ -172,6 +172,10 @@ def initialize_project(self): message = 'A list must be provided when a git operation is specified.' LOGGER.critical(message) raise ValueError(message) + elif not (self.users or self.orgs or self.gists or self.stars or self.view or self.clone or self.pull): + message = 'At least one git operation and one list must be provided to run github-archive.' + LOGGER.critical(message) + raise ValueError(message) def authenticated_user_in_users(self): return self.authenticated_user.login.lower() in self.users @@ -204,7 +208,10 @@ def get_all_git_assets(self, context): LOGGER.debug(f'{formatted_owner_name} {git_asset_string} retrieved!') for item in git_assets: - if self.forks or (self.forks is False and item.fork is False): + if context == GIST_CONTEXT: + # Automatically add gists since we don't support forked gists + all_git_assets.append(item) + elif self.forks or (self.forks is False and item.fork is False): all_git_assets.append(item) else: # Do not include this forked asset @@ -281,13 +288,13 @@ def archive_repo(self, thread_limiter, repo, repo_path, operation): pass else: commands = { - PULL_OPERATION: f'cd {repo_path} && git pull --rebase', + PULL_OPERATION: ['cd', repo_path, '&&', 'git', 'pull', '--rebase'], } if self.use_https: - commands.update({CLONE_OPERATION: f'git clone {repo.html_url} {repo_path}'}) + commands.update({CLONE_OPERATION: ['git', 'clone', repo.html_url, repo_path]}) else: - commands.update({CLONE_OPERATION: f'git clone {repo.ssh_url} {repo_path}'}) + commands.update({CLONE_OPERATION: ['git', 'clone', repo.ssh_url, repo_path]}) git_command = commands[operation] try: @@ -297,7 +304,6 @@ def archive_repo(self, thread_limiter, repo, repo_path, operation): stdout=subprocess.DEVNULL, stdin=subprocess.DEVNULL, stderr=subprocess.DEVNULL, - shell=True, check=True, timeout=self.timeout, ) @@ -319,8 +325,8 @@ def archive_gist(self, thread_limiter, gist, gist_path, operation): pass else: commands = { - CLONE_OPERATION: f'git clone {gist.html_url} {gist_path}', - PULL_OPERATION: f'cd {gist_path} && git pull --rebase', + CLONE_OPERATION: ['git', 'clone', gist.html_url, gist_path], + PULL_OPERATION: ['cd', gist_path, '&&', 'git', 'pull', '--rebase'], } git_command = commands[operation] @@ -331,7 +337,6 @@ def archive_gist(self, thread_limiter, gist, gist_path, operation): stdout=subprocess.DEVNULL, stdin=subprocess.DEVNULL, stderr=subprocess.DEVNULL, - shell=True, check=True, timeout=self.timeout, ) diff --git a/setup.py b/setup.py index 9e26009..7900860 100644 --- a/setup.py +++ b/setup.py @@ -18,7 +18,7 @@ setuptools.setup( name='github-archive', - version='4.1.0', + version='4.1.1', description=( 'A powerful tool to concurrently clone or pull user and org repos and gists to create a GitHub archive.' ), diff --git a/test/unit/test_archive.py b/test/unit/test_archive.py index 27cadf6..22497ff 100644 --- a/test/unit/test_archive.py +++ b/test/unit/test_archive.py @@ -229,7 +229,10 @@ def test_run_stars_pull(mock_get_all_git_assets, mock_iterate_repos_to_archive): @patch('os.path.exists', return_value=False) @patch('os.makedirs') def test_initialize_project(mock_make_dirs, mock_dir_exist): - GithubArchive().initialize_project() + GithubArchive( + users='justintime50', + clone=True, + ).initialize_project() assert mock_make_dirs.call_count == 2 @@ -240,7 +243,9 @@ def test_initialize_project_missing_list(mock_logger): # Parametrize doesn't work great because we can't easily swap the param name being used message = 'A git operation must be specified when a list of users or orgs is provided.' with pytest.raises(ValueError) as error: - GithubArchive(users='justintime50').initialize_project() + GithubArchive( + users='justintime50', + ).initialize_project() mock_logger.critical.assert_called_with(message) assert message == str(error.value) @@ -260,6 +265,18 @@ def test_initialize_project_missing_operation(mock_logger): assert message == str(error.value) +@patch('github_archive.archive.LOGGER') +def test_initialize_project_missing_all_cli_args(mock_logger): + # TODO: Is it possible to test all variations easily in one test? + # Parametrize doesn't work great because we can't easily swap the param name being used + message = 'At least one git operation and one list must be provided to run github-archive.' + with pytest.raises(ValueError) as error: + GithubArchive().initialize_project() + + mock_logger.critical.assert_called_with(message) + assert message == str(error.value) + + @patch('github_archive.archive.Github.get_user') def test_authenticated_user_in_users(mock_get_user): authenticated_user_in_users = GithubArchive( @@ -383,6 +400,20 @@ def test_archive_repo_success(mock_logger, mock_subprocess, mock_git_asset): mock_logger.info.assert_called_once_with(message) +@patch('subprocess.run') +@patch('github_archive.archive.LOGGER') +def test_archive_repo_use_https_success(mock_logger, mock_subprocess, mock_git_asset): + # TODO: Mock the subprocess better to ensure it's doing what it should + operation = CLONE_OPERATION + message = f'Repo: {mock_git_asset.owner.login}/{mock_git_asset.name} {operation} success!' + GithubArchive( + use_https=True, + ).archive_repo(mock_thread_limiter(), mock_git_asset, 'mock/path', operation) + + mock_subprocess.assert_called() + mock_logger.info.assert_called_once_with(message) + + @patch('os.path.exists', return_value=True) @patch('subprocess.run') @patch('github_archive.archive.LOGGER')