Skip to content

Fix regression in up #1858

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

Closed
aanand wants to merge 1 commit into from
Closed

Fix regression in up #1858

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
20 changes: 12 additions & 8 deletions compose/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,26 +245,30 @@ def up(self,
if force_recreate and not allow_recreate:
raise ValueError("force_recreate and allow_recreate are in conflict")

services = self.get_services(service_names, include_deps=start_deps)
services = self.get_services(service_names, include_deps=False)
all_services = self.get_services(service_names, include_deps=start_deps)

for service in services:
for service in all_services:
service.remove_duplicate_containers()

plans = self._get_convergence_plans(
services,
all_services,
allow_recreate=allow_recreate,
force_recreate=force_recreate,
)

return [
container
for service in services
for container in service.execute_convergence_plan(
containers_to_return = []

for service in all_services:
containers = service.execute_convergence_plan(
plans[service.name],
do_build=do_build,
timeout=timeout
)
]
if service in services:
containers_to_return += containers

return containers_to_return
Copy link

@dnephin dnephin Aug 12, 2015

Choose a reason for hiding this comment

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

I'm not sure about this. I think project.up() should still return all the containers it created.

I would think the filtering should happen in the cli.main.up() (or it can call a function to do the filtering) since that's the layer that deals with user expectations.

I think it's conceivable that something else might want to know all the containers created by up (I say this because I've actually written code that has that expectation).

Copy link
Author

@aanand aanand Aug 12, 2015

Choose a reason for hiding this comment

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

Fair enough - only problem with doing it in cli.main.up is that it can't be tested.

Copy link
Author

@aanand aanand Aug 12, 2015

Choose a reason for hiding this comment

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

(easily)

Copy link

@dnephin dnephin Aug 12, 2015

Choose a reason for hiding this comment

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

Ya, that's true. At some point I think it might make sense to split up some of the larger cli functions into some smaller functions (to make it easier to test), but that doesn't need to happen now.

In this case I think everything that happens after project.up() could be in a separate function:

if detached:
    return

print_logs_for_up(containers, service_names, monochrome, timeout)


def _get_convergence_plans(self,
services,
Expand Down
19 changes: 16 additions & 3 deletions tests/integration/project_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,16 +174,29 @@ def test_start_stop_kill_remove(self):
self.assertEqual(len(project.containers(stopped=True)), 0)

def test_project_up(self):
web = self.create_service('web')
db = self.create_service('db', volumes=['/var/db'])
project = Project('composetest', [web, db], self.client)
web = self.create_service('web', links=[(db, 'db')])
project = Project('composetest', [db, web], self.client)
project.start()
self.assertEqual(len(project.containers()), 0)

project.up(['db'])
containers = project.up(['db'])
self.assertEqual(len(project.containers()), 1)
self.assertEqual(len(db.containers()), 1)
self.assertEqual(len(web.containers()), 0)
self.assertEqual(set(containers), set(db.containers()))

containers = project.up(['web'])
self.assertEqual(len(project.containers()), 2)
self.assertEqual(len(db.containers()), 1)
self.assertEqual(len(web.containers()), 1)
self.assertEqual(set(containers), set(web.containers()))

containers = project.up([])
self.assertEqual(len(project.containers()), 2)
self.assertEqual(len(db.containers()), 1)
self.assertEqual(len(web.containers()), 1)
self.assertEqual(set(containers), set(project.containers()))

def test_project_up_starts_uncreated_services(self):
db = self.create_service('db')
Expand Down