Introduce `profiles` by ndeloof · Pull Request #98 · compose-spec/compose-spec

ndeloof · 2020-09-02T07:42:14Z

What this PR does / why we need it:
Introduce profiles in compose schema and specification.
This is based on #97 discussion, but does not cover the override file proposal, which could be proposed in a follow-up PR.

Which issue(s) this PR fixes:
see #97

Signed-off-by: Nicolas De Loof <[email protected]>

rumpl · 2020-09-02T12:00:50Z

+services:
+  foo:
+     image: foo
+  bar:
+     image: bar
+     profiles: ["test"]
+  zot:
+     image: zot
+     profiles: ["debug"]
+     depends_on: ["bar"]


Suggested change

services:

foo:

image: foo

bar:

image: bar

profiles: ["test"]

zot:

image: zot

profiles: ["debug"]

depends_on: ["bar"]

services:

foo:

image: foo

bar:

image: bar

profiles:

- test

zot:

depends_on:

- bar

image: zot

profiles:

- debug

rumpl · 2020-09-02T12:03:34Z

+
+- Compose application model parsed with no profile enabled only contains the `foo` service.
+- If profile `test` is enabled, model contains both the `foo` and `bar` service.
+- If profile `debug` is enabled, model contains both the `foo` and `zot` service, but not `bar` 


Suggested change

- If profile `debug` is enabled, model contains both the `foo` and `zot` service, but not `bar`

- If profile `debug` is enabled, model contains both `foo` and `zot` services, but not `bar`

acran · 2020-09-02T12:49:50Z

+implementation when none of the listed profiles match the active ones, until the target component is
+explicitly targeted by a command.
+
+References to other services (by `links`, `extends` or shared resource syntax `service:xxx`) MUST not


See #97 (comment)

The term "shared resource syntax service:xxx" is not defined anywhere in the spec document.

Where can a newcomer learn what is it? It would be good if there was at least a hyperlink here.

This is referring to system resources shareable between services or with the host, where you can specify the resource with the service:[service name] syntax, see network_mode and ipc in the spec.

Thanks for the clarification. If the document included these links to the spec that would be really helpful for other people, too. Do you think it can be added?

Now I see that this snippet was already merged in https://github.com/compose-spec/compose-spec/pull/110/files#diff-bc6661da34ecae62fbe724bb93fd69b91a7f81143f2683a81163231de7e3b545R182.

My comment should really be placed there. The gist of it stands: could a link network_mode and ipc be added in the document?

acran · 2020-09-02T13:05:31Z

With the follow-up PR for override files in mind: should we maybe narrow down the valid profile names? These should exclude all invalid characters for filesystem paths and maybe also / and \?

Also having a profile name start with - could make parsing command lines hard:

# start profile named "--build"
compose up --profile --build

ndeloof · 2020-09-02T13:21:23Z

About restriction on profile name, I suggest we re-use work done on #60 and define [a-zA-Z0-9][a-zA-Z0-9_.-]+ as a global pattern for "names".

EricHripko

Looks like a great start and a lovely potential addition to the spec 🙌 I got one chunky point of discussion inline and a few minor suggestions 💬

EricHripko · 2020-09-02T17:16:46Z

+References to other services (by `links`, `extends` or shared resource syntax `service:xxx`) MUST not
+automatically enable a component that would otherwise have been ignored by active profiles.
+
+#### illustration sample


Suggested change

#### illustration sample

#### Illustration sample

Since other headings uppercase first letter

EricHripko · 2020-09-02T17:19:55Z


+### profiles
+
+`profiles` defines a list of named profile for volume to be enabled. When not set, volume is always enabled.


Suggested change

`profiles` defines a list of named profile for volume to be enabled. When not set, volume is always enabled.

`profiles` defines a list of named profiles for volume to be enabled under. When not set, volume is always enabled.

EricHripko · 2020-09-02T17:20:06Z


+### profiles
+
+`profiles` defines a list of named profile for network to be enabled. When not set, network is always enabled


Suggested change

`profiles` defines a list of named profile for network to be enabled. When not set, network is always enabled

`profiles` defines a list of named profiles for network to be enabled under. When not set, network is always enabled

EricHripko · 2020-09-02T17:20:15Z


+### profiles
+
+`profiles` defines a list of named profile for service to be enabled. When not set, service is always enabled.


Suggested change

`profiles` defines a list of named profile for service to be enabled. When not set, service is always enabled.

`profiles` defines a list of named profiles for service to be enabled under. When not set, service is always enabled.

EricHripko · 2020-09-02T17:32:51Z

+All top-level elements support a `profiles` attribute to define a list of named profiles. Elements without 
+a `profiles` attribute set MUST always be enabled. The enclosing component MUST be ignored by compose
+implementation when none of the listed profiles match the active ones, until the target component is
+explicitly targeted by a command.


It feels a bit strange that users can get (what is essentially) a one-off profile with always-on services + just the target service. This might get rather confusing since we're ignoring profiles attribute on the target service, but enforcing it everywhere else.

Is it worth to fomalise this a bit more by saying that compose run/up TARGET will activate the profiles of the target service? I appreciate that this probably makes the lives of Compose implementors harder (especially considering the follow-up profile-specific files proposal), but this functionality in isolation seems more confusing than beneficial.

EricHripko · 2020-10-19T17:20:10Z

@ndeloof - have you had a chance to break this out into a smaller initial profile proposal?

ndeloof · 2020-10-20T09:14:28Z

@EricHripko I have been pretty busy, feel free to take ownership on this topic if you can

acran · 2020-10-20T14:23:01Z

@ndeloof I'd gladly support here, what can I do to advance this PR?

AFAIK the next step was to reduce the scope of this PR to a minimum and add further use cases with additional PRs? But for me this PR already is quite limited in scope, so what should be cut for the first step? I would only think of limiting the profiles key word to the service model.

If that would be okay, I could create a new PR with this and incorporate the pending feedback from here (since I can not take over this PR??).

ndeloof · 2020-10-20T14:29:33Z

@acran yes indeed, we agreed on reducing scope to profile attribute introduction with minimal "magic" so we cover some basic use-cases and can have further debates on the more advanced ones.
Feel free to create another PR and cherry-pick any relevant parts from this one

acran · 2020-10-21T20:25:49Z

I created a new PR with hopefully all the feedback incorporated in #110. Please have a look (=

EricHripko · 2020-10-23T08:36:48Z

This is awesome - thanks for picking it up @acran 🙂

ndeloof · 2021-01-28T13:52:11Z

closing as obsolete

Introduce profiles

afd2ebb

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof requested review from EricHripko and rumpl and removed request for EricHripko September 2, 2020 07:42

rumpl reviewed Sep 2, 2020

View reviewed changes

acran reviewed Sep 2, 2020

View reviewed changes

EricHripko reviewed Sep 2, 2020

View reviewed changes

acran mentioned this pull request Oct 21, 2020

Introduce profiles for services #110

Merged

ndeloof closed this Jan 28, 2021

	- If profile `debug` is enabled, model contains both the `foo` and `zot` service, but not `bar`
	- If profile `debug` is enabled, model contains both `foo` and `zot` services, but not `bar`


		### profiles

		`profiles` defines a list of named profile for volume to be enabled. When not set, volume is always enabled.

Uh oh!

Conversation

ndeloof commented Sep 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acran commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndeloof commented Sep 2, 2020

Uh oh!

EricHripko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EricHripko commented Oct 19, 2020

Uh oh!

ndeloof commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acran commented Oct 20, 2020

Uh oh!

ndeloof commented Oct 20, 2020

Uh oh!

acran commented Oct 21, 2020

Uh oh!

EricHripko commented Oct 23, 2020

Uh oh!

ndeloof commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

acran commented Sep 2, 2020 •

edited

Loading

ndeloof commented Oct 20, 2020 •

edited

Loading