Add options to the compose loader by vdemeester · Pull Request #1128 · docker/cli

vdemeester · 2018-06-15T14:19:44Z

Add the possibility to skip interpolation
Add the possibility to skip schema validation
Allow customizing the substitution function, to add special cases.

Example of use.

loader.Load(composetypes.ConfigDetails{
	WorkingDir:  ".",
	ConfigFiles: files,
	Environment: env,
}, func(opts *loader.Options) {
	opts.Interpolate.Substitute = composetemplate.SubstituteWith(template, mapping, errorIfMissing)
})

// […]

func errorIfMissing(substitution string, mapping composetemplate.Mapping) (string, bool, error) {
	value, found := mapping(substitution)
	if !found {
		return "", true, &composetemplate.InvalidTemplateError{
			Template: fmt.Sprintf("required variable %s is missing a value", substitution),
		}
	}
	return value, true, nil
}

Again, this is gonna be very useful for docker/app 👼

Signed-off-by: Vincent Demeester vincent@sbr.pm

silvin-lubecki

I think some unit tests would help here 😄

silvin-lubecki · 2018-06-15T14:36:18Z

cli/compose/template/template.go

-					Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage),
-				}
+		for _, f := range subsFuncs {
+			var value string


nit:

var( value string applied bool )

silvin-lubecki · 2018-06-15T14:38:29Z

cli/compose/template/template.go

+
+// Soft default (fall back if unset or empty)
+func softDefault(substitution string, mapping Mapping) (string, bool, error) {
+	if strings.Contains(substitution, ":-") {


nit:

if !strings.Contains(substitution, ":-"){ return "", false, nil } ...

silvin-lubecki · 2018-06-15T14:38:39Z

cli/compose/template/template.go

+
+// Hard default (fall back if-and-only-if empty)
+func hardDefault(substitution string, mapping Mapping) (string, bool, error) {
+	if strings.Contains(substitution, "-") {


silvin-lubecki · 2018-06-15T14:38:44Z

cli/compose/template/template.go

+}
+
+func requiredNonEmpty(substitution string, mapping Mapping) (string, bool, error) {
+	if strings.Contains(substitution, ":?") {


silvin-lubecki · 2018-06-15T14:38:51Z

cli/compose/template/template.go

+}
+
+func required(substitution string, mapping Mapping) (string, bool, error) {
+	if strings.Contains(substitution, "?") {


silvin-lubecki · 2018-06-15T14:40:54Z

Linter is complaining:

cli/compose/template/template.go:35:1:warning: comment on exported type SubstituteFunc should be of the form "SubstituteFunc ..." (with optional leading article) (golint)

silvin-lubecki · 2018-06-25T12:50:06Z

cli/compose/template/template.go


-// Substitute variables in the string with their values
-func Substitute(template string, mapping Mapping) (string, error) {
+// SubstituteFunc is a user-supplied function that apply substition.


s/substition/substitution/

silvin-lubecki · 2018-06-25T12:50:48Z

cli/compose/template/template.go

-func Substitute(template string, mapping Mapping) (string, error) {
+// SubstituteFunc is a user-supplied function that apply substition.
+// Returns the value as a string, a bool indicating if the function could apply
+// the substition and an error.


s/substition/substitution/

thaJeztah · 2018-06-25T13:16:19Z

cli/compose/loader/loader.go


 // Load reads a ConfigDetails and returns a fully loaded configuration
-func Load(configDetails types.ConfigDetails) (*types.Config, error) {
+func Load(configDetails types.ConfigDetails, ops ...func(*Options)) (*types.Config, error) {


perhaps s/ops/options/ but that's a nit

thaJeztah · 2018-06-25T13:17:26Z

cli/compose/loader/loader.go

+	}
+
+	for _, op := range ops {
+		op(opts)


This seems to be the standard pattern, but was just thinking why functional options cannot produce an error 🤔

@thaJeztah they could 😉 I just though they would not here 😝

thaJeztah · 2018-06-25T13:20:20Z

cli/compose/loader/loader.go

-		if err != nil {
-			return nil, err
+		if !opts.SkipInterpolation {
+			var err error


Can you remove this var err .. ? There's more err variables in this function, so having this here will be confusing.

thaJeztah · 2018-06-25T13:27:44Z

cli/compose/loader/loader.go


-		if err := schema.Validate(configDict, configDetails.Version); err != nil {
-			return nil, err
+		if !opts.SkipValidation {


I guess the reason to allow skipping "validation" is that if "interpolation" is skipped, there's values that are invalid (because, e.g. $port is not a valid port-number)?

Is there a use-case to disable validation or interpolation separately?

Also, trying to think of situations where skipping validation could skip important checks (although, I guess in the end that's the daemon/API's responsibility)

If we don't interpolate, for sure we don't want to validate (otherwise the composefile could be invalid). The other way could be happens, i.e. doing some interpolation but not validating (for whatever reason, like if it's later on used with something else).

Also, trying to think of situations where skipping validation could skip important checks (although, I guess in the end that's the daemon/API's responsibility)

Yes. And also, this is only in the library part of cli/compose — i.e. with this PR it is not possible to not validate or interpolate (and we shouldn't allow that with the docker cli) 😉

thaJeztah · 2018-06-25T13:31:49Z

cli/compose/template/template.go

+type SubstituteFunc func(string, Mapping) (string, bool, error)
+
+// SubstituteWith subsitute variables in the string with their values.
+// It accepts additionnal substitute function.


s/additionnal/additional/

thaJeztah · 2018-06-25T13:32:57Z

cli/compose/template/template.go

+// SubstituteWith subsitute variables in the string with their values.
+// It accepts additionnal substitute function.
+func SubstituteWith(template string, mapping Mapping, pattern *regexp.Regexp, subsFuncs ...SubstituteFunc) (string, error) {
+	subsFuncs = append([]SubstituteFunc{


Should we always append the default functions, or only do so if no custom functions are provided?

Good point. I did that to not export the default function, but looking back at it, this function should probably just take the list and use them (letting user decide which one to apply)

thaJeztah · 2018-06-25T13:36:00Z

cli/compose/template/template.go

+				applied bool
+			)
+			value, applied, err = f(substitution, mapping)
+			if err != nil {


Is the error itself discarded?

@thaJeztah it's not if it's the last one to be applied (i.e. if one function doesn't apply, or apply with error, it doesn't necessarly mean others wont).

- Add the possibility to skip interpolation - Add the possibility to skip schema validation - Allow customizing the substitution function, to add special cases. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

thaJeztah

LGTM

vdemeester requested review from dnephin, silvin-lubecki and thaJeztah June 15, 2018 14:19

GordonTheTurtle added the status/0-triage label Jun 15, 2018

vdemeester added status/2-code-review and removed status/0-triage labels Jun 15, 2018

vdemeester force-pushed the interpolation-options branch from f57d7df to 53f3114 Compare June 15, 2018 14:32

silvin-lubecki requested changes Jun 15, 2018

View reviewed changes

vdemeester force-pushed the interpolation-options branch 2 times, most recently from 06c6486 to de77f86 Compare June 15, 2018 15:03

vdemeester mentioned this pull request Jun 19, 2018

Allow . in variables used for interpolation #1127

Closed

vdemeester force-pushed the interpolation-options branch from de77f86 to 3367be8 Compare June 25, 2018 12:39

silvin-lubecki reviewed Jun 25, 2018

View reviewed changes

vdemeester force-pushed the interpolation-options branch 4 times, most recently from f05c771 to f8ffc8e Compare June 25, 2018 12:59

silvin-lubecki approved these changes Jun 25, 2018

View reviewed changes

thaJeztah requested changes Jun 25, 2018

View reviewed changes

vdemeester mentioned this pull request Jun 25, 2018

Update docker/cli with upstream changes docker-archive-public/docker.app#244

Merged

vdemeester force-pushed the interpolation-options branch from f8ffc8e to 468e96d Compare June 25, 2018 15:13

Add options to the compose loader

9fdd14f

- Add the possibility to skip interpolation - Add the possibility to skip schema validation - Allow customizing the substitution function, to add special cases. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the interpolation-options branch from 468e96d to 9fdd14f Compare June 25, 2018 15:15

thaJeztah approved these changes Jun 26, 2018

View reviewed changes

silvin-lubecki merged commit 204ab4c into docker:master Jun 26, 2018

GordonTheTurtle added this to the 18.06.0 milestone Jun 26, 2018

silvin-lubecki deleted the interpolation-options branch June 26, 2018 13:05

SMFloris mentioned this pull request Dec 16, 2020

Add stack config command #2740

Closed

Conversation

vdemeester commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silvin-lubecki 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!

silvin-lubecki commented Jun 15, 2018

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!

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!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vdemeester commented Jun 15, 2018 •

edited

Loading