diff --git a/src/runner/__init__.py b/src/runner/__init__.py index 470b22b..60c2bdf 100644 --- a/src/runner/__init__.py +++ b/src/runner/__init__.py @@ -14,24 +14,22 @@ def setuser(): os.setuid(pwd.getpwnam('customer').pw_uid) -def run(logger, command, cwd=None, env=None, shell=False, check=False, node=False, ruby=False): +def run(logger, command, cwd=None, env=None, shell=False, check=True, node=False, ruby=False): ''' Run an OS command with provided cwd or env, stream logs to logger, and return the exit code. Errors that occur BEFORE the command is actually executed are caught and handled here. - Errors encountered by the executed command are NOT caught. Instead a non-zero exit code - will be returned to be handled by the caller. + Errors encountered by the executed command are caught unless `check=False`. In these cases a + non-zero exit code will be returned to be handled by the caller. See https://docs.python.org/3/library/subprocess.html#popen-constructor for details. ''' - # TODO - refactor to put the appropriate bundler binaries in PATH so this isn't necessary if ruby: command = f'source {RVM_PATH} && {command}' shell = True - # TODO - refactor to put the appropriate node/npm binaries in PATH so this isn't necessary if node: command = f'source {NVM_PATH} && {command}' shell = True diff --git a/src/steps/build.py b/src/steps/build.py index 9eb1fad..423753d 100644 --- a/src/steps/build.py +++ b/src/steps/build.py @@ -114,7 +114,8 @@ def check_supported_ruby_version(version): f'ruby -e "exit Gem::Version.new(\'{shlex.split(version)[0]}\') >= Gem::Version.new(\'{RUBY_VERSION_MIN}\') ? 1 : 0"', # noqa: E501 cwd=CLONE_DIR_PATH, env={}, - ruby=True + ruby=True, + check=False, ) upgrade_msg = 'Please upgrade to an actively supported version, see https://www.ruby-lang.org/en/downloads/branches/ for details.' # noqa: E501 @@ -141,7 +142,7 @@ def setup_node(should_cache: bool, bucket, s3_client, post_metrics, status_callb logger = get_logger('setup-node') def runp(cmd): - return run(logger, cmd, cwd=CLONE_DIR_PATH, env={}, check=True, node=True) + return run(logger, cmd, cwd=CLONE_DIR_PATH, env={}, node=True) NVMRC_PATH = CLONE_DIR_PATH / NVMRC if NVMRC_PATH.is_file(): @@ -213,7 +214,8 @@ def run_build_script(branch, owner, repository, site_prefix, logger = get_logger(f'run-{script_name}-script') logger.info(f'Running {script_name} build script in package.json') env = build_env(branch, owner, repository, site_prefix, base_url, user_env_vars) - return run(logger, f'npm run {script_name}', cwd=CLONE_DIR_PATH, env=env, node=True) + run(logger, f'npm run {script_name}', cwd=CLONE_DIR_PATH, env=env, node=True) + return def download_hugo(): @@ -254,8 +256,8 @@ def download_hugo(): hugo_tar.write(chunk) HUGO_BIN_PATH = WORKING_DIR_PATH / HUGO_BIN - run(logger, f'tar -xzf {hugo_tar_path} -C {WORKING_DIR_PATH}', env={}, check=True) - run(logger, f'chmod +x {HUGO_BIN_PATH}', env={}, check=True) + run(logger, f'tar -xzf {hugo_tar_path} -C {WORKING_DIR_PATH}', env={}) + run(logger, f'chmod +x {HUGO_BIN_PATH}', env={}) return except Exception: failed_attempts += 1 @@ -276,7 +278,7 @@ def build_hugo(branch, owner, repository, site_prefix, HUGO_BIN_PATH = WORKING_DIR_PATH / HUGO_BIN - run(logger, f'echo hugo version: $({HUGO_BIN_PATH} version)', env={}, check=True) + run(logger, f'echo hugo version: $({HUGO_BIN_PATH} version)', env={}) logger.info('Building site with hugo') @@ -285,7 +287,7 @@ def build_hugo(branch, owner, repository, site_prefix, hugo_args += f' --baseURL {base_url}' env = build_env(branch, owner, repository, site_prefix, base_url, user_env_vars) - run(logger, f'{HUGO_BIN_PATH} {hugo_args}', cwd=CLONE_DIR_PATH, env=env, node=True, check=True) + run(logger, f'{HUGO_BIN_PATH} {hugo_args}', cwd=CLONE_DIR_PATH, env=env, node=True) def setup_ruby(should_cache, post_metrics, status_callback_url): @@ -297,7 +299,7 @@ def setup_ruby(should_cache, post_metrics, status_callback_url): logger = get_logger('setup-ruby') def runp(cmd): - return run(logger, cmd, cwd=CLONE_DIR_PATH, env={}, ruby=True, check=True) + return run(logger, cmd, cwd=CLONE_DIR_PATH, env={}, ruby=True) RUBY_VERSION_PATH = CLONE_DIR_PATH / RUBY_VERSION if RUBY_VERSION_PATH.is_file(): @@ -320,7 +322,7 @@ def setup_bundler(should_cache: bool, bucket, s3_client): logger = get_logger('setup-bundler') def runp(cmd): - return run(logger, cmd, cwd=CLONE_DIR_PATH, env={}, ruby=True, check=True) + return run(logger, cmd, cwd=CLONE_DIR_PATH, env={}, ruby=True) GEMFILE_PATH = CLONE_DIR_PATH / GEMFILE GEMFILELOCK_PATH = CLONE_DIR_PATH / GEMFILELOCK @@ -424,7 +426,6 @@ def build_jekyll(branch, owner, repository, site_prefix, f'echo Building using Jekyll version: $({jekyll_cmd} -v)', cwd=CLONE_DIR_PATH, env={}, - check=True, ruby=True ) @@ -437,6 +438,5 @@ def build_jekyll(branch, owner, repository, site_prefix, cwd=CLONE_DIR_PATH, env=env, node=True, - ruby=True, - check=True + ruby=True ) diff --git a/test/test_build.py b/test/test_build.py index 2735af2..060e3d7 100644 --- a/test/test_build.py +++ b/test/test_build.py @@ -78,7 +78,7 @@ def test_installs_deps(self, mock_get_logger, mock_run, patch_clone_dir): ]) def callp(cmd): - return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, check=True, node=True) + return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, node=True) mock_run.assert_has_calls([ callp('echo Node version: $(node --version)'), @@ -118,9 +118,7 @@ def test_it_runs_federalist_script_when_it_exists(self, mock_get_logger, mock_ru base_url='/site/prefix' ) - result = run_build_script(**kwargs) - - assert result == mock_run.return_value + run_build_script(**kwargs) mock_get_logger.assert_called_once_with('run-federalist-script') @@ -155,9 +153,7 @@ def test_it_runs_pages_script_when_it_exists(self, mock_get_logger, mock_run, base_url='/site/prefix' ) - result = run_build_script(**kwargs) - - assert result == mock_run.return_value + run_build_script(**kwargs) mock_get_logger.assert_called_once_with('run-pages-script') @@ -234,7 +230,7 @@ def test_no_ruby_version_file(self, mock_check_supported_ruby_version, mock_post_metrics.assert_called_once() def callp(cmd): - return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True, check=True) + return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True) mock_run.assert_has_calls([ callp('ruby -v'), @@ -258,7 +254,7 @@ def test_it_uses_ruby_version_if_it_exists(self, mock_logger = mock_get_logger.return_value def callp(cmd): - return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True, check=True) + return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True) mock_run.assert_has_calls([ callp(f'rvm install {version}'), @@ -282,7 +278,7 @@ def test_it_strips_and_quotes_ruby_version(self, mock_logger = mock_get_logger.return_value def callp(cmd): - return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True, check=True) + return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True) mock_logger.info.assert_has_calls([ call('Using ruby version in .ruby-version'), @@ -365,7 +361,7 @@ def test_when_no_gemfile_just_load_jekyll(self, mock_get_logger, mock_run, patch mock_run.assert_called_once_with( mock_logger, 'gem install jekyll -v 4.2.2 --no-document', - cwd=patch_clone_dir, env={}, ruby=True, check=True + cwd=patch_clone_dir, env={}, ruby=True ) def test_it_uses_default_version_if_only_gemfile_exits(self, mock_get_logger, @@ -387,7 +383,7 @@ def test_it_uses_default_version_if_only_gemfile_exits(self, mock_get_logger, ]) def callp(cmd): - return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True, check=True) + return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True) mock_run.assert_has_calls([ callp(f'gem install bundler --version "{default_version}"'), @@ -416,7 +412,7 @@ def test_it_uses_bundler_version_if_gemfile_and_bundler_file_exists(self, mock_g ]) def callp(cmd): - return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True, check=True) + return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True) mock_run.assert_has_calls([ callp(f'gem install bundler --version "{version}"'), @@ -457,7 +453,6 @@ def test_with_no_gemfile(self, mock_get_logger, mock_run, patch_clone_dir, f'echo Building using Jekyll version: $({command} -v)', cwd=patch_clone_dir, env={}, - check=True, ruby=True, ), call( @@ -467,7 +462,6 @@ def test_with_no_gemfile(self, mock_get_logger, mock_run, patch_clone_dir, env=env, node=True, ruby=True, - check=True, ) ]) @@ -501,7 +495,6 @@ def test_with_gemfile(self, mock_get_logger, mock_run, patch_clone_dir, patch_si f'echo Building using Jekyll version: $({command} -v)', cwd=patch_clone_dir, env={}, - check=True, ruby=True, ), call( @@ -511,7 +504,6 @@ def test_with_gemfile(self, mock_get_logger, mock_run, patch_clone_dir, patch_si env=env, node=True, ruby=True, - check=True, ) ]) @@ -565,8 +557,8 @@ def test_it_is_callable(self, mock_get_logger, mock_run, patch_working_dir, patc ]) mock_run.assert_has_calls([ - call(mock_logger, tar_cmd, env={}, check=True), - call(mock_logger, chmod_cmd, env={}, check=True) + call(mock_logger, tar_cmd, env={}), + call(mock_logger, chmod_cmd, env={}) ]) def test_it_is_callable_retry(self, mock_get_logger, mock_run, patch_working_dir, @@ -607,8 +599,8 @@ def test_it_is_callable_retry(self, mock_get_logger, mock_run, patch_working_dir ]) mock_run.assert_has_calls([ - call(mock_logger, tar_cmd, env={}, check=True), - call(mock_logger, chmod_cmd, env={}, check=True) + call(mock_logger, tar_cmd, env={}), + call(mock_logger, chmod_cmd, env={}) ]) def test_it_is_exception(self, mock_get_logger, mock_run, patch_working_dir, patch_clone_dir): @@ -687,7 +679,6 @@ def test_it_calls_hugo_as_expected(self, mock_get_logger, mock_run, mock_logger, f'echo hugo version: $({hugo_path} version)', env={}, - check=True ), call( mock_logger, @@ -695,7 +686,6 @@ def test_it_calls_hugo_as_expected(self, mock_get_logger, mock_run, cwd=patch_clone_dir, env=build_env(*kwargs.values()), node=True, - check=True ) ]) @@ -804,7 +794,7 @@ def test_it_uses_ruby_cache_when_gemfile_lock(self, mock_sp_run, mock_cache_fold mock_cache_folder.assert_called_once() def callp(cmd): - return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True, check=True) + return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, ruby=True) mock_run.assert_has_calls([ callp(f'gem install bundler --version "{default_version}"'), @@ -832,7 +822,7 @@ def test_it_uses_node_cache_when_package_lock(self, mock_sp_run, mock_cache_fold mock_cache_folder.assert_called_once() def callp(cmd): - return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, check=True, node=True) + return call(mock_logger, cmd, cwd=patch_clone_dir, env={}, node=True) mock_run.assert_has_calls([ callp('echo Node version: $(node --version)'), diff --git a/test/test_runner.py b/test/test_runner.py index ee6492b..042cf5f 100644 --- a/test/test_runner.py +++ b/test/test_runner.py @@ -13,7 +13,7 @@ def test_run(mock_popen): mock_popen.return_value = Mock(returncode=0, stdout=Mock(readline=Mock(return_value='foobar'))) - result = run(mock_logger, command) + run(mock_logger, command) mock_popen.assert_called_once_with( shlex.split(command), @@ -31,9 +31,6 @@ def test_run(mock_popen): mock_logger.info.assert_called_once_with('foobar') - assert result == 0 - - @patch('subprocess.Popen', autospec=True) def test_run_popen_failure(mock_popen): mock_logger = Mock() @@ -41,7 +38,8 @@ def test_run_popen_failure(mock_popen): mock_popen.side_effect = ValueError('ugh') - result = run(mock_logger, command) + with raises(ValueError): + run(mock_logger, command) mock_popen.assert_called_once_with( shlex.split(command), @@ -60,18 +58,18 @@ def test_run_popen_failure(mock_popen): mock_logger.error.assert_any_call('Encountered a problem invoking Popen.') mock_logger.error.assert_any_call('ugh') - assert result == 1 - @patch('subprocess.Popen', autospec=True) -def test_run_popen_failure_check_true(mock_popen): +def test_run_popen_failure_check_false(mock_popen): mock_logger = Mock() command = 'foobar' + return_code = 1 mock_popen.side_effect = ValueError('ugh') - with raises(ValueError, match='ugh'): - run(mock_logger, command, check=True) + result = run(mock_logger, command, check=False) + + assert result == return_code mock_popen.assert_called_once_with( shlex.split(command), @@ -92,14 +90,14 @@ def test_run_popen_failure_check_true(mock_popen): @patch('subprocess.Popen', autospec=True) -def test_run_popen_output_check_true(mock_popen): +def test_run_popen_output(mock_popen): mock_logger = Mock() command = 'foobar' string_output = 'string_output' mock_popen.return_value = Mock(returncode=0, stdout=Mock(readline=Mock(return_value=string_output))) # noqa: E501 - result = run(mock_logger, command, check=True) + result = run(mock_logger, command) assert result == string_output mock_popen.assert_called_once_with( @@ -118,13 +116,13 @@ def test_run_popen_output_check_true(mock_popen): @patch('subprocess.Popen', autospec=True) -def test_run_os_failure(mock_popen): +def test_run_os_failure_check_false(mock_popen): mock_logger = Mock() command = 'foobar' mock_popen.side_effect = OSError('ugh') - result = run(mock_logger, command) + result = run(mock_logger, command, check=False) mock_popen.assert_called_once_with( shlex.split(command), @@ -156,7 +154,7 @@ def test_run_os_failure_check_true(mock_popen): mock_popen.side_effect = OSError('ugh') with raises(OSError, match='ugh'): - run(mock_logger, command, check=True) + run(mock_logger, command) mock_popen.assert_called_once_with( shlex.split(command), @@ -179,14 +177,14 @@ def test_run_os_failure_check_true(mock_popen): @patch('subprocess.Popen', autospec=True) -def test_run_command_failure(mock_popen): +def test_run_command_failure_check_false(mock_popen): mock_logger = Mock() command = 'foobar' return_code = 2 mock_popen.return_value = Mock(returncode=return_code, stdout=Mock(readline=Mock(return_value='text'))) # noqa: E501 - result = run(mock_logger, command) + result = run(mock_logger, command, check=False) mock_popen.assert_called_once_with( shlex.split(command), @@ -214,7 +212,7 @@ def test_run_command_failure_check_true(mock_popen): mock_popen.return_value = Mock(returncode=return_code, stdout=Mock(readline=Mock(return_value='text'))) # noqa: E501 with raises(subprocess.CalledProcessError): - run(mock_logger, command, check=True) + run(mock_logger, command) mock_popen.assert_called_once_with( shlex.split(command), @@ -288,6 +286,6 @@ def test_access_environ(): command = 'cat /proc/1/environ' env = {} - run(mock_logger, command, env=env) + run(mock_logger, command, env=env, check=False) mock_logger.info.assert_any_call('cat: /proc/1/environ: Permission denied')