Skip to content

extends must be copied from override file #2206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mnowster merged 1 commit into from
Oct 20, 2015
Merged

extends must be copied from override file #2206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions compose/config/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,9 @@ def load_file(filename, config):
def merge_services(base, override):
all_service_names = set(base) | set(override)
return {
name: merge_service_dicts(base.get(name, {}), override.get(name, {}))
name: merge_service_dicts_from_files(
base.get(name, {}),
override.get(name, {}))
for name in all_service_names
}

Expand Down Expand Up @@ -270,9 +272,7 @@ def validate_and_construct_extends(self):
extends,
self.filename
)
self.extended_config_path = self.get_extended_config_path(
extends
)
self.extended_config_path = self.get_extended_config_path(extends)
self.extended_service_name = extends['service']

config = load_yaml(self.extended_config_path)
Expand Down Expand Up @@ -355,6 +355,17 @@ def process_container_options(service_dict, working_dir=None):
return service_dict


def merge_service_dicts_from_files(base, override):
"""When merging services from multiple files we need to merge the `extends`
field. This is not handled by `merge_service_dicts()` which is used to
perform the `extends`.
"""
new_service = merge_service_dicts(base, override)
if 'extends' in override:
new_service['extends'] = override['extends']
return new_service


def merge_service_dicts(base, override):
d = base.copy()

Expand Down
37 changes: 37 additions & 0 deletions tests/unit/config/config_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,43 @@ def test_load_with_multiple_files_and_empty_base(self):
config.load(details)
assert 'Top level object needs to be a dictionary' in exc.exconly()

def test_load_with_multiple_files_and_extends_in_override_file(self):
base_file = config.ConfigFile(
'base.yaml',
{
'web': {'image': 'example/web'},
})
override_file = config.ConfigFile(
'override.yaml',
{
'web': {
'extends': {
'file': 'common.yml',
'service': 'base',
},
'volumes': ['/home/user/project:/code'],
},
})
details = config.ConfigDetails('.', [base_file, override_file])

tmpdir = py.test.ensuretemp('config_test')
Copy link
Copy Markdown

@mnowster mnowster Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not immediately clear why you're doing this? How does creating a temporary directory fit into the description of what this test is doing around extends? Could this have been achieved with a fixture?

Copy link
Copy Markdown
Author

@dnephin dnephin Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this have been achieved with a fixture?

If we go with the py.test way of doing fixtures, maybe. With unittest fixtures, no.

Fixtures are just "common test setup", but there is always other test setup that isn't all that common. This is just setting up the environment so the test exercises the expected functionality.

It's creating a common.yml which is referenced from the config above.

I'm not sure how to make this more clear.

Copy link
Copy Markdown
Author

@dnephin dnephin Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more detail to the PR description about why this is different

tmpdir.join('common.yml').write("""
base:
labels: ['label=one']
""")
with tmpdir.as_cwd():
service_dicts = config.load(details)

expected = [
{
'name': 'web',
'image': 'example/web',
'volumes': ['/home/user/project:/code'],
'labels': {'label': 'one'},
},
]
self.assertEqual(service_sort(service_dicts), service_sort(expected))

def test_config_valid_service_names(self):
for valid_name in ['_', '-', '.__.', '_what-up.', 'what_.up----', 'whatup']:
config.load(
Expand Down