feat: Add label_file support to service #713
Conversation
Signed-off-by: Suleiman Dibirov <[email protected]>
|
@ndeloof hi! What do you think about the PR, should I continue implementing the feature like I do(it uses the same way as we load env_file) or should I do it in a different way? |
|
Sounds good to me. I'd prefer we can rely on docker/cli parser for this file so we don't have 2 implementations, but this would mean we resolve labels in docker/compose, and on the other hand format is pretty minimalist, so let's go this way |
Signed-off-by: Suleiman Dibirov <[email protected]>
Signed-off-by: Suleiman Dibirov <[email protected]>
Signed-off-by: Suleiman Dibirov <[email protected]>
There was a problem hiding this comment.
LGTM
in this PR, you're anticipating the need to set a label_file as required. Do you have a use-case in mind ? We introduced this for env_file as a workaround for corner-case usages, but I'd prefer we don't make things unnecessary complicated if we don't have to, at least for a first iteration.
Signed-off-by: Suleiman Dibirov <[email protected]>
I did it to follow the current logic for the env_file. removed it + added tests |
Signed-off-by: Suleiman Dibirov <[email protected]>
| "label_file": { | ||
| "oneOf": [ | ||
| {"type": "string"}, | ||
| { |
There was a problem hiding this comment.
Sounds to me this is a premature feature to add support for required here. label_file should be declared at most a string|array of string for user convenience
There was a problem hiding this comment.
@ndeloof agree, fixed
Signed-off-by: Suleiman Dibirov <[email protected]>
override/uncity.go
Outdated
| switch value := y.(type) { | ||
| case string: | ||
| return value, nil | ||
| case map[string]any: |
There was a problem hiding this comment.
not relevant anymore AFAICT
transform/labelfile.go
Outdated
| } | ||
| } | ||
|
|
||
| func transformLabelFileValue(data any) any { |
| @@ -217,10 +217,24 @@ func services(workingDir, homeDir string) types.Services { | |||
| Ipc: "host", | |||
| Uts: "host", | |||
| Labels: map[string]string{ | |||
| "FOO": "foo_from_label_file", | |||
There was a problem hiding this comment.
FYI this huge test we inherited from legacy compose parser in docker/cli.
As it is painful to maintain, we prefer to have individual test checking specific attribute is well supported (which you also provided 🥳)
There was a problem hiding this comment.
should I revert these changes or keep them?
Signed-off-by: Suleiman Dibirov <[email protected]>
|
@ndeloof pushed changes, now label_file has type |
schema/compose-spec.json
Outdated
| "format": { | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
is this removal intentional ?
There was a problem hiding this comment.
yes, you wrote to make it string or []string, so I did it).
There was a problem hiding this comment.
sure, but here you removed format attribute from env_file defintion
There was a problem hiding this comment.
oh, accidentally deleted it, put it back
types/labelfile.go
Outdated
| "encoding/json" | ||
| ) | ||
|
|
||
| type LabelFile struct { |
There was a problem hiding this comment.
yes, thank you, removed
Signed-off-by: Suleiman Dibirov <[email protected]>
Signed-off-by: Suleiman Dibirov <[email protected]>
Issue: docker/compose#12311