Skip to content
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.

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

Use container_name as prefix if explicitly defined #6476

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@collin5
Copy link
Contributor

collin5 commented Jan 20, 2019

Even when container_name is prefixed with the project name i.e baz_foo for project baz

Resolves #6442

collin5 added some commits Jan 20, 2019

thread project config through LogPresenter
Signed-off-by: Collins Abitekaniza <abtcolns@gmail.com>
use container_name as prefix if explicitly defined
Signed-off-by: Collins Abitekaniza <abtcolns@gmail.com>
add tests for logs prefix
Signed-off-by: Collins Abitekaniza <abtcolns@gmail.com>
@collin5

This comment has been minimized.

Copy link
Contributor Author

collin5 commented Apr 9, 2019

return '{prefix} {line}'.format(
prefix=self.color_func(prefix + ' |'),
line=line)


def build_log_presenters(service_names, monochrome):
def build_log_presenters(service_names, monochrome, config=None):

This comment has been minimized.

Copy link
@chris-crone

chris-crone Apr 9, 2019

Contributor

The prefix_width can no longer be calculated just from the service_names as the longest name could now be a container_name.

As part of this change, we may be able to avoid passing the whole config to __init__.

@@ -238,8 +240,9 @@ class TopLevelCommand(object):
version Show the Docker-Compose version information
"""

def __init__(self, project, options=None):
def __init__(self, project, config=None, options=None):

This comment has been minimized.

Copy link
@chris-crone

chris-crone Apr 9, 2019

Contributor

Please put new arguments at the end as this ensures that people using the arguments positionally aren't broken.

@chris-crone

This comment has been minimized.

Copy link
Contributor

chris-crone commented Apr 9, 2019

Thanks for the PR @collin5!

I left some feedback but I think we may need to do more refactoring to make this change cleanly. I'm not a huge fan of passing the config around as much as we have to here to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.