fix: bug fixes and improvements (closes #40)

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`

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,
                 )

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.'
     ),

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')