Feature/support labels #26

huguesmcneilduval · 2021-11-25T05:33:26Z

Changes Description

Added support for labels. A spring config server path is "app/profiles/label" . https://cloud.spring.io/spring-cloud-config/reference/html/#_locating_remote_configuration_resources

Flatting the configuration in a "map" way.

From

{
 "my.config1": "value",
 "my.config2": "value2"
}

to

{
 "my": {
    "config1": "value,
    "config2": "value2"
  }
}

Associated Issues

I needed the labels and a get_property function to retrieve "my.config1" value for example.

Affected Files

See commits

Unit Tests Changed or Added

I've added tests for all files

Let me know if this interested you. I'll will probably add more functionality to this lib as I go along.

…ath":"value2" to "my.config: path:value, secondpath:value2. Its hard to describe in git commit, but Im flatting the config in a yaml way

* "feature/support-labels" * Transforming configs like "my.config.path":"value, "my.config.secondpath":"value2" to "my.config: path:value, secondpath:value2. Its hard to describe in git commit, but Im flatting the config in a yaml way

Piszmog · 2021-11-25T17:39:36Z

client.go

@@ -4,11 +4,12 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"github.com/Piszmog/cfservices"
-	"golang.org/x/oauth2/clientcredentials"


Please revert this. For this project, import formatting uses gofmt instead of goimports.

Hopefully imports formats get figured out (see #37719)

ok so if I understand well, goimports is a superset of gofmt. gofmt format struct, field and whatnot while goimports add format for imports?
I've manually reverted the import to what they were, I can't seem to use gofmt to reformat struct.

My understanding of reading on the web is that goimports is just better than gofmt.

Piszmog · 2021-11-25T17:39:58Z

client_test.go

@@ -3,12 +3,13 @@ package cloudconfigclient_test
 import (
 	"context"
 	"errors"
-	"github.com/Piszmog/cloudconfigclient/v2"
-	"github.com/stretchr/testify/require"
-	"golang.org/x/oauth2/clientcredentials"


Piszmog · 2021-11-25T17:41:51Z

configuration.go

@@ -52,6 +56,18 @@ func (s *Source) HandlePropertySources(handler PropertySourceHandler) {
 	}
 }

+func (s *Source) Get(key string, defaultValue string) interface{} {


Done.

Also this is where the Map I've added could be used. If we use the map to retrieve the config value, we would have complexity O(1) instead of O(n) on every search right. Therefore I think this lib should use the Map as it's core.

Piszmog · 2021-11-25T17:42:53Z

configuration.go

+func (s *Source) Get(key string, defaultValue string) interface{} {
+	for _, propertySource := range s.PropertySources {
+		for _key, value := range propertySource.Source {
+			fmt.Printf("key=%s, value=%v\n", _key, value)


the library should avoid unnecessary logging/noise. Please remove this

Piszmog · 2021-11-25T17:43:25Z

configuration.go

@@ -52,6 +56,18 @@ func (s *Source) HandlePropertySources(handler PropertySourceHandler) {
 	}
 }

+func (s *Source) Get(key string, defaultValue string) interface{} {
+	for _, propertySource := range s.PropertySources {
+		for _key, value := range propertySource.Source {


rename _key to sourceKey to make it more clear. I was confused at first what _key was

Piszmog · 2021-11-25T17:44:38Z

go.mod

@@ -1,4 +1,4 @@
-module github.com/Piszmog/cloudconfigclient/v2
+module github.com/duvalhub/cloudconfigclient


Piszmog · 2021-11-25T17:46:19Z

configuration.go

@@ -1,8 +1,11 @@
 package cloudconfigclient


General Formatting needs to be done. Please use gofmt

Piszmog

See comments

Piszmog · 2021-11-25T17:54:17Z

So the flatness is from the Config Server itself. The key values are in a Properties format. Looking at the code and your initial PR comment, I would recommend a function that converts Properties format to the structure that you want, vs what you have now.

For example,

func (p *PropertySource) FromProperties() map[string]interface{} {
  ...
}

This is where we would unflatten the key values

Piszmog · 2021-11-25T17:55:36Z

configuration.go

+	return dest
+}
+
+type BasicAuthTransport struct {


this bit and down seems redundant and is more application specific implementation of this library. I do not think it belong in this library.

I've added this BasicAuthTransport because this lib does not support Basic Authentication with ConfigServer (unless I missed something). This bit of code handle that.

huguesmcneilduval · 2021-12-01T02:59:51Z

So the flatness is from the Config Server itself. The key values are in a Properties format. Looking at the code and your initial PR comment, I would recommend a function that converts Properties format to the structure that you want, vs what you have now.

For example,
func (p *PropertySource) FromProperties() map[string]interface{} {
  ...
}
This is where we would unflatten the key values

My idea is that the returned format from Config Server is a bit useless. What gives to loop through all PropertySource when we can access the value straight in a map. It's also less efficient. Like the function func (s *Source) Get(key string, defaultValue string) interface{} is not efficient at the moment. I feel like this whole lib should be based on the Map insteand of on

Also my plan (or my used case) is to be able to map a config key to a struct, like this :
structObject := &StructObject{} json := source.Data["path.to.config.dict"] mapstructure.Decode(json, &structObject )
This is a application specific but by experience, I know that we always want to consume configuration by mapping them to a struct (or java class, or python dataclass, etc). I would like to have a function in this lib that can do what @ConfigurationProperties does basically. Give a struct type and a key path to config and get your struct back populated with the values in the conf.

huguesmcneilduval · 2021-12-01T01:59:47Z

client.go

@@ -4,11 +4,12 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"github.com/Piszmog/cfservices"
-	"golang.org/x/oauth2/clientcredentials"


ok so if I understand well, goimports is a superset of gofmt. gofmt format struct, field and whatnot while goimports add format for imports?
I've manually reverted the import to what they were, I can't seem to use gofmt to reformat struct.

My understanding of reading on the web is that goimports is just better than gofmt.

huguesmcneilduval · 2021-12-01T02:00:18Z

client_test.go

@@ -3,12 +3,13 @@ package cloudconfigclient_test
 import (
 	"context"
 	"errors"
-	"github.com/Piszmog/cloudconfigclient/v2"
-	"github.com/stretchr/testify/require"
-	"golang.org/x/oauth2/clientcredentials"


huguesmcneilduval · 2021-12-01T02:08:35Z

configuration.go

@@ -52,6 +56,18 @@ func (s *Source) HandlePropertySources(handler PropertySourceHandler) {
 	}
 }

+func (s *Source) Get(key string, defaultValue string) interface{} {


Done.

Also this is where the Map I've added could be used. If we use the map to retrieve the config value, we would have complexity O(1) instead of O(n) on every search right. Therefore I think this lib should use the Map as it's core.

huguesmcneilduval · 2021-12-01T02:09:12Z

configuration.go

@@ -52,6 +56,18 @@ func (s *Source) HandlePropertySources(handler PropertySourceHandler) {
 	}
 }

+func (s *Source) Get(key string, defaultValue string) interface{} {
+	for _, propertySource := range s.PropertySources {
+		for _key, value := range propertySource.Source {


huguesmcneilduval · 2021-12-01T02:09:21Z

configuration.go

+func (s *Source) Get(key string, defaultValue string) interface{} {
+	for _, propertySource := range s.PropertySources {
+		for _key, value := range propertySource.Source {
+			fmt.Printf("key=%s, value=%v\n", _key, value)


huguesmcneilduval · 2021-12-01T02:10:45Z

configuration.go

+	return dest
+}
+
+type BasicAuthTransport struct {


I've added this BasicAuthTransport because this lib does not support Basic Authentication with ConfigServer (unless I missed something). This bit of code handle that.

huguesmcneilduval · 2021-12-01T02:11:17Z

configuration.go

+	return dest
+}
+
+func insertInMapRecursion(s []string, value interface{}, dest map[string]interface{}) map[string]interface{} {


insertInMap and insertInMapRecursion have the same functional logic. We should keep one of the two.

huguesmcneilduval · 2021-12-01T02:43:14Z

go.mod

@@ -1,4 +1,4 @@
-module github.com/Piszmog/cloudconfigclient/v2
+module github.com/duvalhub/cloudconfigclient


sonarcloud · 2021-12-01T03:06:54Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

Piszmog · 2022-02-27T22:13:16Z

Hi, sorry for taking a long time to get back to you on this. I have add the functionality to unmarshal the ProeprtySource slice in Source to a struct that you provide.

See #27 for an example

huguesmcneilduval added 3 commits November 20, 2021 19:21

"feature/support-labels"

b589647

Transforming configs like "my.config.path":"value, "my.config.secondp…

4f66c39

…ath":"value2" to "my.config: path:value, secondpath:value2. Its hard to describe in git commit, but Im flatting the config in a yaml way

Not much

c257af2

huguesmcneilduval requested a review from Piszmog as a code owner November 25, 2021 05:33

Feature/support labels (#1)

ce72942

* "feature/support-labels" * Transforming configs like "my.config.path":"value, "my.config.secondpath":"value2" to "my.config: path:value, secondpath:value2. Its hard to describe in git commit, but Im flatting the config in a yaml way

Piszmog reviewed Nov 25, 2021

View reviewed changes

configuration.go

@@ -1,8 +1,11 @@

package cloudconfigclient

Copy link

Owner

Piszmog Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General Formatting needs to be done. Please use gofmt

Piszmog requested changes Nov 25, 2021

View reviewed changes

Piszmog reviewed Nov 25, 2021

View reviewed changes

huguesmcneilduval added 2 commits November 30, 2021 22:00

Changes link to review

11badaf

Merge branch 'main' into feature/support-labels

4de7821

huguesmcneilduval commented Dec 1, 2021

View reviewed changes

huguesmcneilduval requested a review from Piszmog December 1, 2021 03:03

huguesmcneilduval added 2 commits November 30, 2021 22:06

Continue...

9865dd6

Continue...

687f6e4

Piszmog closed this Feb 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/support labels #26

Feature/support labels #26

huguesmcneilduval commented Nov 25, 2021 •

edited

Loading

Piszmog Nov 25, 2021

huguesmcneilduval Dec 1, 2021

Piszmog Nov 25, 2021

huguesmcneilduval Dec 1, 2021

Piszmog Nov 25, 2021

huguesmcneilduval Dec 1, 2021

Piszmog Nov 25, 2021

huguesmcneilduval Dec 1, 2021

Piszmog Nov 25, 2021

huguesmcneilduval Dec 1, 2021

Piszmog Nov 25, 2021

huguesmcneilduval Dec 1, 2021

Piszmog Nov 25, 2021

Piszmog left a comment

Piszmog commented Nov 25, 2021 •

edited

Loading

Piszmog Nov 25, 2021

huguesmcneilduval Dec 1, 2021

huguesmcneilduval commented Dec 1, 2021 •

edited

Loading

huguesmcneilduval Dec 1, 2021

huguesmcneilduval Dec 1, 2021

huguesmcneilduval Dec 1, 2021

huguesmcneilduval Dec 1, 2021

huguesmcneilduval Dec 1, 2021

huguesmcneilduval Dec 1, 2021

huguesmcneilduval Dec 1, 2021

huguesmcneilduval Dec 1, 2021

sonarcloud bot commented Dec 1, 2021

Piszmog commented Feb 27, 2022

		@@ -1,4 +1,4 @@
		module github.com/Piszmog/cloudconfigclient/v2
		module github.com/duvalhub/cloudconfigclient

Feature/support labels #26

Feature/support labels #26

Conversation

huguesmcneilduval commented Nov 25, 2021 • edited Loading

Changes Description

Associated Issues

Affected Files

Unit Tests Changed or Added

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

Piszmog left a comment

Choose a reason for hiding this comment

Piszmog commented Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huguesmcneilduval commented Dec 1, 2021 • 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

sonarcloud bot commented Dec 1, 2021

Piszmog commented Feb 27, 2022

huguesmcneilduval commented Nov 25, 2021 •

edited

Loading

Piszmog commented Nov 25, 2021 •

edited

Loading

huguesmcneilduval commented Dec 1, 2021 •

edited

Loading