Skip to content

Commit

Permalink
Fix for config file overriding CLI logging option
Browse files Browse the repository at this point in the history
Because config was being loaded after the CLI option processing, not before it,
the config file value of loglevel superseded the CLI option. This is the wrong
ordering, but to fix it, I had to move the config loading into a setting in the
execctx. Once that was done, usage sites for the config data change to lookups
in the execctx to find their config data.
Also broke some tests initially because they mock over the top of the wrong
config object. Updates these tests to (generally speaking) ignore the config
object handling and leave it to the behaviors of the test containment classes.
  • Loading branch information
sirosen committed Nov 7, 2015
1 parent f6bfa53 commit 1f21310
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 21 deletions.
10 changes: 3 additions & 7 deletions salve/block/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,14 @@ def __init__(self, file_context, source=None):
self.min_attrs.add('source')
self.primary_attr = 'source'

def expand_blocks(self, root_dir, config, v3_relpaths, ancestors=None):
def expand_blocks(self, root_dir, v3_relpaths, ancestors=None):
"""
Expands a manifest block by reading its source, parsing it into
blocks, and assigning those to be the sub_blocks of the manifest
block, forming a block tree. This is, in a certain sense, part
of the parser.
Args:
@config is used to fill in any variable values in the
blocks' template string attributes.
@root_dir is the root of all relative paths in the manifest.
@v3_relpaths
Expand All @@ -68,7 +65,7 @@ def expand_blocks(self, root_dir, config, v3_relpaths, ancestors=None):
from salve.parser import parse_stream
# ensure that this block has config applied and paths expanded
# this guarantees that 'source' is accurate
config.apply_to_block(self)
ExecutionContext()['config'].apply_to_block(self)
self.expand_file_paths(root_dir)
self.ensure_has_attrs('source')
filename = self['source']
Expand All @@ -94,14 +91,13 @@ def expand_blocks(self, root_dir, config, v3_relpaths, ancestors=None):
# recursively apply to manifest blocks
if isinstance(b, ManifestBlock):
b.expand_blocks(containing_dir,
config,
v3_relpaths,
ancestors=ancestors)
# expand any relative paths and substitute for any vars
# must be in order so that a variable which expands to a
# relative path works correctly
else:
config.apply_to_block(b)
ExecutionContext()['config'].apply_to_block(b)
b.expand_file_paths(containing_dir)

def compile(self):
Expand Down
8 changes: 1 addition & 7 deletions salve/cli/deploy.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import salve

from salve import paths
from salve.config import SALVEConfig
from salve.context import FileContext
from salve.exceptions import SALVEException
from salve.block import ManifestBlock
Expand All @@ -27,11 +26,6 @@ def run_on_manifest(root_manifest, args):
@args
The options, as parsed from the commandline.
"""
cfg_file = None
if args.configfile:
cfg_file = args.configfile
conf = SALVEConfig(filename=cfg_file)

root_dir = paths.containing_dir(root_manifest)
if args.directory and not args.v3_relpath:
root_dir = os.path.abspath(args.directory)
Expand All @@ -40,7 +34,7 @@ def run_on_manifest(root_manifest, args):
# manifest
root_block = ManifestBlock(FileContext('no such file'),
source=root_manifest)
root_block.expand_blocks(root_dir, conf, args.v3_relpath)
root_block.expand_blocks(root_dir, args.v3_relpath)

root_action = root_block.compile()
root_action(ConcreteFilesys())
Expand Down
6 changes: 6 additions & 0 deletions salve/cli/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import textwrap

import salve
from salve.config import SALVEConfig
from salve.context import ExecutionContext
import salve.cli.deploy
import salve.cli.backup

Expand Down Expand Up @@ -112,6 +114,10 @@ def load_args():
parser = get_parser()
args = parser.parse_args()

# load and store config data from config file (if present)
conf = SALVEConfig(filename=args.configfile)
ExecutionContext()['config'] = conf

if args.log_level:
salve.logger.setLevel(salve.log.str_to_level(args.log_level))

Expand Down
15 changes: 10 additions & 5 deletions tests/unit/block/manifest_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from tests.util import ensure_except, full_path

from salve import paths
from salve.context import ExecutionContext
from salve.exceptions import BlockException

from salve.block import ManifestBlock, FileBlock
Expand All @@ -17,6 +18,10 @@ def _man_block_and_containing_dir(name):


class TestWithScratchdir(ScratchWithExecCtx):
def setUp(self):
ScratchWithExecCtx.setUp(self)
ExecutionContext()['config'] = dummy_conf

@istest
def sourceless_manifest_compile_error(self):
"""
Expand All @@ -35,7 +40,7 @@ def sourceless_manifest_expand_error(self):
are expanded if the source attribute is unspecified.
"""
b = ManifestBlock(dummy_file_context)
ensure_except(BlockException, b.expand_blocks, '/', dummy_conf, False)
ensure_except(BlockException, b.expand_blocks, '/', False)

@istest
def empty_manifest_expand(self):
Expand All @@ -45,7 +50,7 @@ def empty_manifest_expand(self):
errors.
"""
b, d = _man_block_and_containing_dir('empty.manifest')
b.expand_blocks('/', dummy_conf, False)
b.expand_blocks('/', False)
assert len(b.sub_blocks) == 0

@istest
Expand All @@ -56,7 +61,7 @@ def recursive_manifest_error(self):
BlockException when expanded.
"""
b, d = _man_block_and_containing_dir('self_include.manifest')
ensure_except(BlockException, b.expand_blocks, d, dummy_conf, False)
ensure_except(BlockException, b.expand_blocks, d, False)

@istest
def sub_block_expand(self):
Expand All @@ -65,7 +70,7 @@ def sub_block_expand(self):
Verifies that Manifest block expansion works normally.
"""
b, d = _man_block_and_containing_dir('empty_and_file.manifest')
b.expand_blocks(d, dummy_conf, False)
b.expand_blocks(d, False)
assert len(b.sub_blocks) == 2
mblock = b.sub_blocks[0]
fblock = b.sub_blocks[1]
Expand All @@ -85,7 +90,7 @@ def sub_block_compile(self):
conversion works normally.
"""
b, d = _man_block_and_containing_dir('empty_and_file.manifest')
b.expand_blocks(d, dummy_conf, False)
b.expand_blocks(d, False)

act = b.compile()

Expand Down
3 changes: 1 addition & 2 deletions tests/unit/cli/deploy_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,9 @@ def __init__(self):
self.ctx = dummy_file_context

@istest
@mock.patch('salve.cli.deploy.SALVEConfig')
@mock.patch('salve.config.SALVEConfig')
@mock.patch('salve.cli.deploy.ManifestBlock')
def deploy_main(self, mock_man, mock_config, mock_deploy_config):
def deploy_main(self, mock_man, mock_config):
"""
Unit: Deploy Command Dummy Manifest Block Expand And Run
Checks that running the deploy main function expands and runs
Expand Down

0 comments on commit 1f21310

Please sign in to comment.