Add Environment Variable Source to Staert #42

serbrech · 2018-03-02T14:25:03Z

Description

This is a first shot at providing an environment variable source to staert (and traefik). I revived #30, extended it, and finished it. I'll add documentation (it's pretty much similar to the initial PR one, so see :
I tested it briefly with traefik, I'll send a PR there too as I get closer to a releasable state.)
I have integrated it in traefik to test, and it manages to parse the entire config tree from traefik. that covers many cases. I'm not certain it manages to SET all values though. so more tests will be needed there.

Quite some work left to do though before calling it done :

Make sure the priority order is ok when running envsource + toml + cli
Handle defaultPointerConfig
Handle some edge cases
Log correctly (info/debug..)
Document it. (see 9b6531c)
Cleanup
Refactor (not pretty code to my standards, but it's a good start)
Make the env variables discoverable (list all env variable name for a given config struct, maybe separate PR)
Bonus: Rewrite it, it's not efficient, there are better ways to do it 😄

DefaultPointerConfig

When it comes to defaultPointerConfig, some notes that made me understand it 🥇

it's used for optional defaults.
if you dont set any of the values under that pointer, then you take the value from the defaultConfig
if you set just --pointerName, it touches the value, and so, you take the default pointer value
if you set any of the parameter under that pointer, you use the defaultPointerConfig value as default

Related to traefik/traefik#7

coveralls · 2018-03-02T20:42:20Z

Coverage increased (+2.3%) to 82.504% when pulling 6d1fcfb on serbrech:envsource-PR into e4d686c on containous:master.

ldez

It's a first quick review.

I did not repeat a message for the repetitive problems.
I do not take a look on algorithmic.

ldez · 2018-03-02T21:36:14Z

envsource_assign_test.go

+	"testing"
+
+	"github.com/containous/flaeg/parse"
+


please group imports.

import ( "reflect" "testing" "github.com/containous/flaeg/parse" "github.com/stretchr/testify/assert" )

ldez · 2018-03-02T21:37:01Z

envsource_assign_test.go

+				OtherStringValue string
+			}{},
+			[]*envValue{
+				&envValue{"FOO", path{"StringValue"}},


&envValue declaration is not needed, same everywhere

ldez · 2018-03-02T21:38:34Z

envsource.go

+
+	values, err := e.analyzeStruct(configVal.Type(), []string{})
+
+	if err != nil {


values, err := e.analyzeStruct(configVal.Type(), []string{}) if err != nil { return err }

ldez · 2018-03-02T21:39:36Z

envsource.go

+// Recursively scan the given config structure type information
+// and look for defined environment variables.
+func (e *envSource) analyzeStruct(configType reflect.Type, currentPath path) ([]*envValue, error) {
+	res := []*envValue{}


var res []*envValue

ldez · 2018-03-02T21:40:18Z

envsource.go

+
+			if err != nil {
+				return []*envValue{}, err
+			}


values, err := e.analyzeValue(field.Type, currentPath) if err != nil { return nil, err }

ldez · 2018-03-02T21:55:37Z

envsource_assign_test.go

+				t.Fail()
+			}
+
+			assert.Exactly(t, testCase.Expectation, testCase.Value)


Don't mix stblib test and testify.

Either you use stblib everywhere or testify, but not the both.

ldez · 2018-03-02T21:56:17Z

envsource_assign_test.go

+	}
+}
+
+func TestFilterEnvVarWithPrefix(t *testing.T) {


could move the tests on the top of the file (after type declaration)

ldez · 2018-03-02T21:57:34Z

envsource_assign_test.go

+		Expectation interface{}
+	}{
+		{
+			"BasicStruct",


could you explicitly name all fields.

ldez · 2018-03-02T21:58:17Z

envsource_assign_test.go

+				Config []basicAppConfig
+			}{},
+			[]*envValue{
+				&envValue{"Test", path{"Config", "0", "StringValue"}},


&envValue declaration is not needed, same everywhere

ldez · 2018-03-02T21:59:20Z

envsource_assign_test.go

+	expectedPtrPtr := getPtrPtrConfig()
+
+	testCases := []struct {
+		Label       string


add the test cases description for each test.
use a field desc

That's what the Label field should do. "BasicStructPointerPointer", "WithWrongPath". What format do you use for test names/descriptions?

We use desc to avoid ambiguities of names like name, label, ...

The description must be a sentence or partial sentence.
ex: with a wrong path

I love the should ... when ... format.

Fixed the label to provide better context and be more readable. if a test fails it looks like this :
TestAssignValues/should_assign_values_to_config_with_a_map_that_uses_a_parser_for_values (0.00s)
changed field name to desc

serbrech · 2018-03-07T21:33:32Z

Commits 9840c06 and onwards are fixes following the review

ldez · 2018-03-11T20:54:21Z

There is a problem, it's complex to explain.

The problem is related to the DefaultPointersConfig management.

To explain I will give you some example from the TomlSource.

Case 1

DurationField = 28

[PtrStruct1]
S1Int = 28

config := &StructPtr{
	PtrStruct1: &Struct1{
		S1Int:    1,
		S1String: "S1StringInitConfig",
	},
	DurationField: parse.Duration(time.Second),
}

defaultPointersConfig := &StructPtr{
	PtrStruct1: &Struct1{
		S1Int:    11,
		S1String: "S1StringDefaultPointersConfig",
		S1Bool:   true,
		S1PtrStruct3: &Struct3{
			S3Float64: 11.11,
		},
	},
}

Result:

{
  "PtrStruct1": {
      "S1Int": 28,
      "S1String": "S1StringDefaultPointersConfig",
      "S1Bool": true,
      "S1PtrStruct3": null
    },
    "DurationField":"28s"
}

Case 2

DurationField = 28

[PtrStruct1]
S1Int = 28
S1String =  "S1StringToml"

config := &StructPtr{
	PtrStruct1: &Struct1{
		S1Int:    1,
		S1String: "S1StringInitConfig",
	},
	DurationField: parse.Duration(time.Second),
}

defaultPointersConfig := &StructPtr{
	PtrStruct1: &Struct1{
		S1Int:    11,
		S1String: "S1StringDefaultPointersConfig",
		S1Bool:   true,
		S1PtrStruct3: &Struct3{
			S3Float64: 11.11,
		},
	},
}

Result:

{
  "PtrStruct1": {
      "S1Int": 28,
      "S1String": "S1StringToml",
      "S1Bool": true,
      "S1PtrStruct3": null
    },
    "DurationField":"28s"
}

Case 3

DurationField = 28

[PtrStruct1]
S1Int = 28
S1Bool = false

config := &StructPtr{
	PtrStruct1: &Struct1{
		S1Int:    1,
		S1String: "S1StringInitConfig",
	},
	DurationField: parse.Duration(time.Second),
}

defaultPointersConfig := &StructPtr{
	PtrStruct1: &Struct1{
		S1Int:    11,
		S1String: "S1StringDefaultPointersConfig",
		S1Bool:   true,
		S1PtrStruct3: &Struct3{
			S3Float64: 11.11,
		},
	},
}

Result:

{
  "PtrStruct1": {
      "S1Int": 28,
      "S1String": "S1StringDefaultPointersConfig",
      "S1Bool": false,
      "S1PtrStruct3": null
    },
    "DurationField":"28s"
}

Case 4

DurationField = 28

[PtrStruct1]
S1Int = 28
S1Bool = false

config := &StructPtr{
	PtrStruct1: &Struct1{
		S1Int:    1,
		S1String: "S1StringInitConfig",
	},
	DurationField: parse.Duration(time.Second),
}

defaultPointersConfig := &StructPtr{
	PtrStruct1: &Struct1{
		S1Int:    11,
		S1String: "S1StringDefaultPointersConfig",
		S1Bool:   true,
		S1PtrStruct3: &Struct3{
			S3Float64: 11.11,
		},
	},
}

Result:

{
  "PtrStruct1": {
      "S1Int": 28,
      "S1String": "S1StringDefaultPointersConfig",
      "S1Bool": false,
      "S1PtrStruct3": null
    },
    "DurationField":"28s"
}

ldez self-requested a review March 2, 2018 14:26

ldez added enhancement WIP labels Mar 2, 2018

ldez suggested changes Mar 2, 2018

View reviewed changes

serbrech force-pushed the envsource-PR branch from 4034604 to 6e12bb4 Compare March 6, 2018 05:24

serbrech mentioned this pull request Mar 8, 2018

EnvSource Documentation #44

Closed

serbrech force-pushed the envsource-PR branch from 9ad4562 to 7c5b730 Compare March 9, 2018 22:01

ldez force-pushed the envsource-PR branch from 7c5b730 to 261e2db Compare March 10, 2018 10:37

ldez removed the WIP label Mar 10, 2018

ldez force-pushed the envsource-PR branch from b0ed2e9 to 0d26b38 Compare March 10, 2018 14:05

ldez force-pushed the envsource-PR branch from 88131b1 to ca3d767 Compare March 12, 2018 14:00

add envsource from existing PR containous#30

5bfee0a

ldez force-pushed the envsource-PR branch from ca3d767 to 5bfee0a Compare March 13, 2018 15:47

serbrech and others added 2 commits March 15, 2018 01:49

adding EnvSource Documentation

825833c

refactor: reorder test cases fields.

6d1fcfb

jjcollinge mentioned this pull request Jul 18, 2018

Passing env specific config file jjcollinge/traefik-on-service-fabric#68

Open

serbrech closed this Feb 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Environment Variable Source to Staert #42

Add Environment Variable Source to Staert #42

serbrech commented Mar 2, 2018 •

edited

Loading

coveralls commented Mar 2, 2018 •

edited

Loading

ldez left a comment •

edited

Loading

ldez Mar 2, 2018 •

edited

Loading

ldez Mar 2, 2018

ldez Mar 2, 2018

ldez Mar 2, 2018

ldez Mar 2, 2018

ldez Mar 2, 2018

ldez Mar 2, 2018

ldez Mar 2, 2018

ldez Mar 2, 2018

ldez Mar 2, 2018

serbrech Mar 5, 2018

ldez Mar 5, 2018

serbrech Mar 8, 2018

serbrech commented Mar 7, 2018

ldez commented Mar 11, 2018


		values, err := e.analyzeStruct(configVal.Type(), []string{})

		if err != nil {

Add Environment Variable Source to Staert #42

Add Environment Variable Source to Staert #42

Conversation

serbrech commented Mar 2, 2018 • edited Loading

Description

DefaultPointerConfig

coveralls commented Mar 2, 2018 • edited Loading

ldez left a comment • edited Loading

Choose a reason for hiding this comment

ldez Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serbrech commented Mar 7, 2018

ldez commented Mar 11, 2018

Case 1

Case 2

Case 3

Case 4

serbrech commented Mar 2, 2018 •

edited

Loading

coveralls commented Mar 2, 2018 •

edited

Loading

ldez left a comment •

edited

Loading

ldez Mar 2, 2018 •

edited

Loading