Implement service profiles by acran · Pull Request #7930 · docker/compose

acran · 2020-11-15T15:41:39Z

Implement profiles as introduced in compose-spec/compose-spec#110

This is a first draft and probably needs some more discussion. Things I'm not quite sure about yet which may need further discussion:

How to specify profiles to enable

by parameter: I implemented the --profile flag for the main command, multiple profiles may be passed
- use another flag name?
- move the flag to the affected subcommands only (see below)?
by environment: I used COMPOSE_PROFILE with : as separator for multiple profiles
- use another variable name?
- use another separator(s)?
- make the separator configurable?
merging profiles: to be consistent with how other array options are handled already the profiles of a service from multiple configuration files are just added together
- there is no way to remove profiles from a service or unset the profiles option all together with override configurations. Should there be?

Affected commands

The main logic is implemented in Project.get_services() which was already used in the code paths for and therefor add profiles support to:

build
create
up
start
pull
push

The following functions use Project.containers() instead which is unchanged and therefor ignore profiles:

stop
down
- using remove_stopped which already used Project.containers()
- using remove_images which I changed to ignore profiles to be consistent with the rest
pause
unpause
kill

I couldn't quite decide if the latter commands should be consider profiles, too, because the behavior expected by the user would most probably not consider them, most notably with docker-compose down:

Executing docker-compose down the user expects that all defined services will be removed. But services assigned to specific profiles only could be missed here when docker-compose down is called without enabling those profiles.
Then those services would need to be considered orphans. But this would be inconsistent with notion of orphans since those services are actually defined in the configuration, just not enabled.
On the other hand docker-compose --profile foo down might suggest that services with the foo profile only would be removed, but actually the spec would require all services without profiles and services with the foo profile.

That's why I thought it to be most consistent/intuitive for docker-compose down and the other commands to just ignore profiles and act on all defined services (and because changing the logic of Project.containers() would require the refactoring/verification of a whole lot more code paths and tests).

Tests

Using the example docker-compose.yml from compose-spec/compose-spec#110 I implemented a test case for each of the examples in the spec as well as an extra test case for merging profiles from multiple configuration files.
Are there more tests needed here?

Small disclaimer: Since I'm neither familiar with the code base nor python even, please don't hold back with any feedback on the coding style/formatting/performance :)

Resolves #7919, #1896, #6742, #7539

acran

I comment on some code lines which probably require some attention

acran · 2020-11-15T15:44:46Z

+                        'active profiles. '
+                        'You may fix this by adding a common profile to '
+                        '"{dep_name}" and "{service_name}".'
+                        .format(dep_name=dep.name, service_name=service.name)


Is this a suitable error message making it clear to the user what went wrong, why and how to fix it?

acran · 2020-11-15T15:48:23Z


    def remove_images(self, remove_image_type):
-        for service in self.get_services():
+        for service in self.services:


The call to self.get_services was actually unnecessary before, since without any parameters it would just return self.services.
But now it would filter services by enabled profiles, so this was changed so remove_images and thus down would act on all defined services.

acran · 2020-11-15T15:49:30Z


    def restart(self, service_names=None, **options):
+        # filter service_names by enabled profiles
+        service_names = [s.name for s in self.get_services(service_names)]


restart should consider the enabled profiles so to not start services disabled by profile with docker-compose restart

aiordache · 2020-12-01T09:24:53Z

@acran Can you rebase please?

acran · 2020-12-01T13:05:39Z

@aiordache yes, done

aiordache

Thank you! SGTM

ulyssessouza

LGTM in general, just missing changes

Implement profiles as introduced in compose-spec/compose-spec#110 fixes docker#7919 closes docker#1896 closes docker#6742 closes docker#7539 Signed-off-by: Roman Anasal <[email protected]>

acran · 2020-12-02T00:25:53Z

I updated the PR again with the feedback from @ulyssessouza. For this I did a little refactoring and created service.enabled_for_profiles() to make the code a bit more readable.

ulyssessouza

LGTM

Thanks @acran !

Adds docs for the new service profiles feature introduced in docker/compose#7930

Adds docs for the new service profiles feature introduced in docker/compose#7930 Co-authored-by: Chris Crone <[email protected]>

acran commented Nov 15, 2020

View reviewed changes

acran force-pushed the profiles branch from 01e01d9 to e02bba4 Compare November 16, 2020 09:45

acran force-pushed the profiles branch from e02bba4 to bc2e56f Compare November 30, 2020 22:40

acran force-pushed the profiles branch from bc2e56f to e2b323c Compare December 1, 2020 13:05

aiordache approved these changes Dec 1, 2020

View reviewed changes

aiordache added this to the 1.28.0 milestone Dec 1, 2020

ulyssessouza suggested changes Dec 1, 2020

View reviewed changes

Comment thread compose/cli/command.py Outdated

Comment thread compose/project.py Outdated

Comment thread compose/project.py Outdated

Comment thread compose/project.py

Comment thread compose/project.py Outdated

Comment thread compose/cli/command.py

acran force-pushed the profiles branch from e2b323c to a5e5318 Compare December 1, 2020 23:54

Implement service profiles

2d2a8a0

Implement profiles as introduced in compose-spec/compose-spec#110 fixes docker#7919 closes docker#1896 closes docker#6742 closes docker#7539 Signed-off-by: Roman Anasal <[email protected]>

acran force-pushed the profiles branch from a5e5318 to 2d2a8a0 Compare December 2, 2020 00:08

ulyssessouza self-requested a review December 2, 2020 12:42

ulyssessouza approved these changes Dec 2, 2020

View reviewed changes

aiordache marked this pull request as ready for review December 2, 2020 18:56

aiordache requested a review from ndeloof as a code owner December 2, 2020 18:56

aiordache merged commit 5e3708e into docker:master Dec 2, 2020

acran added a commit to acran/docker.github.io that referenced this pull request Dec 3, 2020

Add documentation for Compose service profiles

59e731d

Adds docs for the new service profiles feature introduced in docker/compose#7930

acran mentioned this pull request Dec 3, 2020

Add documentation for Compose service profiles docker/docs#11844

Merged

albers mentioned this pull request Jan 22, 2021

Add bash completion for profiles and a command to list profiles #8061

Merged

acran added a commit to acran/docker.github.io that referenced this pull request Jan 25, 2021

Add documentation for Compose service profiles

80ffdca

Adds docs for the new service profiles feature introduced in docker/compose#7930 Co-authored-by: Chris Crone <[email protected]>

acran added a commit to acran/docker.github.io that referenced this pull request Jan 25, 2021

Add documentation for Compose service profiles

ca88315

Adds docs for the new service profiles feature introduced in docker/compose#7930 Co-authored-by: Chris Crone <[email protected]>

acran mentioned this pull request Apr 29, 2022

--profile attribute not respected during 'down' #8139

Closed

acran mentioned this pull request Jul 12, 2022

Add auto_up option to exclude service from docker-compose up by default #7548

Closed

Uh oh!

Conversation

acran commented Nov 15, 2020

How to specify profiles to enable

Affected commands

Tests

Uh oh!

acran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acran Nov 15, 2020

Choose a reason for hiding this comment

Uh oh!

acran Nov 15, 2020

Choose a reason for hiding this comment

Uh oh!

acran Nov 15, 2020

Choose a reason for hiding this comment

Uh oh!

aiordache commented Dec 1, 2020

Uh oh!

acran commented Dec 1, 2020

Uh oh!

aiordache left a comment

Choose a reason for hiding this comment

Uh oh!

ulyssessouza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

acran commented Dec 2, 2020

Uh oh!

ulyssessouza left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants