Skip to content

Commit

Permalink
switch check default
Browse files Browse the repository at this point in the history
  • Loading branch information
drewbo committed May 3, 2024
1 parent e334f13 commit fba4bdc
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 61 deletions.
8 changes: 3 additions & 5 deletions src/runner/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 12 additions & 12 deletions src/steps/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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():
Expand Down Expand Up @@ -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():
Expand Down Expand Up @@ -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
Expand All @@ -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')

Expand All @@ -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):
Expand All @@ -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():
Expand All @@ -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
Expand Down Expand Up @@ -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
)

Expand All @@ -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
)
40 changes: 15 additions & 25 deletions test/test_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)'),
Expand Down Expand Up @@ -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')

Expand Down Expand Up @@ -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')

Expand Down Expand Up @@ -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'),
Expand All @@ -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}'),
Expand All @@ -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'),
Expand Down Expand Up @@ -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,
Expand All @@ -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}"'),
Expand Down Expand Up @@ -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}"'),
Expand Down Expand Up @@ -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(
Expand All @@ -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,
)
])

Expand Down Expand Up @@ -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(
Expand All @@ -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,
)
])

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -687,15 +679,13 @@ 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,
hugo_call,
cwd=patch_clone_dir,
env=build_env(*kwargs.values()),
node=True,
check=True
)
])

Expand Down Expand Up @@ -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}"'),
Expand Down Expand Up @@ -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)'),
Expand Down
Loading

0 comments on commit fba4bdc

Please sign in to comment.