design(Refactor Registry): Create a design for refactoring the Registry component #2133

lenny-goodell · 2019-11-13T18:35:39Z

Design for refactoring the Registry component to separate out the Service Configuration and Service Registration.

Also create initial adr folder and README for location to hold Architecture Decision Records.

Note that this design doesn't address the configuration hierarchy that @jpwhitemn brought up in the PHX F2F. I feel that is out of scope for this effort and should be addressed separately.

mkbhanda · 2019-11-15T04:30:42Z

adr/Registry Refactoring Design.md

+If **no value** is provided to the `-cp/-configProvider` option, i.e. just `-cp`, and no environment variable override is specified, the default value of `consul.http://localhost:8500` will be used. 
+
+if `-cp/-configProvider` **not used** and no environment variable override is specified the local configuration file is used, as is it now.
+


what happens if the local configuration file has no cp entry? use the consul.http://localhost:8500?

A little concerned if services start-up out of sync, that is one referring to a registry foo and another to bar.
Guess an error message may work that attempts a connection check to registry and if it fails, emit an misconfiguration error message and promptly exit.

A la dependency injection, would it make sense to

Plan is to not put config Provider in the local config (chicken before the egg...) If no -cp then config is simply from local file.

mkbhanda

Minor comments added

mkbhanda · 2019-11-15T04:32:29Z

adr/Registry Refactoring Design.md

+	GetConfiguration(configStruct interface{}) (interface{}, error)
+	WatchForChanges(updateChannel chan<- interface{}, errorChannel chan<- error,
+                    configuration interface{}, waitKey string)
+	IsAlive() bool


IsAlive feels more like registry related?

IsAlive() just determines if the Provider is alive, i.e. available. Applicable to Config and Registry

iain-anderson · 2019-11-15T10:40:48Z

adr/Registry Refactoring Design.md

+```go
+type Client interface {
+	HasConfiguration() (bool, error)
+	PutConfigurationToml(configuration *toml.Tree, overwrite bool) error


Do we need this? If we have a toml-backed implementation of the Configuration Client, the configuration in toml.Tree form won't need to be seen outside of that

Yes, it is used by Config Seed since it only deals with the TOML

Does this mean update the configurations from TOML file? If it does, we had better rename it to:
PutConfigurationByToml or PutConfigurationFromToml

Also, if it is only for Config Seed, it might not be put in this interface. Config Seed can convert to configStruct on its own and call PutConfiguration.

iain-anderson · 2019-11-15T10:49:42Z

adr/Registry Refactoring Design.md

+if `-cp/-configProvider` **not used** and no environment variable override is specified the local configuration file is used, as is it now.
+
+The existing `-r/-registry` command line option will be removed from all services. 
+


Is the -p/-profile option being removed also?

Nope. Still need it.

codecov-io · 2019-11-15T19:22:10Z

Codecov Report

Merging #2133 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2133   +/-   ##
=======================================
  Coverage   39.39%   39.39%           
=======================================
  Files         174      174           
  Lines       12812    12812           
=======================================
  Hits         5047     5047           
  Misses       7498     7498           
  Partials      267      267

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf52b33...d9a53a5. Read the comment docs.

…ry component Design for refactoring the Registry component to separate out the Service Configuration and Service Registration. Also create initial `adr` folder and README for location to hold Architecture Decision Records. Signed-off-by: lenny <leonard.goodell@intel.com>

- Added that config provider information will be logged - Changed Stem to Basepath in the New Configuration Client Config - Added section for Config endpoint chnages to include the config provider information Signed-off-by: lenny <leonard.goodell@intel.com>

cloudxxx8

LGTM, it's a nice work.

Only a minor suggestion in the comment, and we had better write down the GODOC to prevent any function name confusion.

cloudxxx8 · 2019-12-08T05:12:26Z

adr/Registry Refactoring Design.md

+```go
+type Client interface {
+	HasConfiguration() (bool, error)
+	PutConfigurationToml(configuration *toml.Tree, overwrite bool) error


Does this mean update the configurations from TOML file? If it does, we had better rename it to:
PutConfigurationByToml or PutConfigurationFromToml

Also, if it is only for Config Seed, it might not be put in this interface. Config Seed can convert to configStruct on its own and call PutConfiguration.

lenny-goodell · 2019-12-10T16:09:33Z

@cloudxxx8 , PutConfigurationToml is currently only used by Config-seed, but from a module perspective it should stay in the interface. I do like the name change for clarity. Thx!

…ry component Design for refactoring the Registry component to separate out the Service Configuration and Service Registration. Also create initial `adr` folder and README for location to hold Architecture Decision Records. Signed-off-by: lenny <leonard.goodell@intel.com>

- Added that config provider information will be logged - Changed Stem to Basepath in the New Configuration Client Config - Added section for Config endpoint chnages to include the config provider information Signed-off-by: lenny <leonard.goodell@intel.com>

…uration Client interface Signed-off-by: lenny <leonard.goodell@intel.com>

…ex-go into RefactorRegistry # Conflicts: # adr/Registry Refactoring Design.md

…uration Client interface Signed-off-by: lenny <leonard.goodell@intel.com>

lenny-goodell requested review from jpwhitemn, tsconn23, anonymouse64, cloudxxx8, iain-anderson, rsdmike, SteveOss, TomFlem and tonyespy November 14, 2019 18:11

mkbhanda reviewed Nov 15, 2019

View reviewed changes

mkbhanda suggested changes Nov 15, 2019

View reviewed changes

iain-anderson reviewed Nov 15, 2019

View reviewed changes

lenny-goodell force-pushed the RefactorRegistry branch 2 times, most recently from 274249a to d4fe39b Compare November 15, 2019 18:32

anonymouse64 removed their request for review November 21, 2019 23:05

lenny-goodell force-pushed the RefactorRegistry branch from d4fe39b to 1024cbd Compare December 4, 2019 18:38

lenny added 2 commits December 6, 2019 14:55

Updates from Design Review

d9a53a5

- Added that config provider information will be logged - Changed Stem to Basepath in the New Configuration Client Config - Added section for Config endpoint chnages to include the config provider information Signed-off-by: lenny <leonard.goodell@intel.com>

lenny-goodell force-pushed the RefactorRegistry branch from e5dc45f to d9a53a5 Compare December 6, 2019 21:56

lenny-goodell marked this pull request as ready for review December 6, 2019 21:56

cloudxxx8 previously approved these changes Dec 8, 2019

View reviewed changes

cloudxxx8 mentioned this pull request Dec 8, 2019

Apply Registry component refactoring in device-sdk-go edgexfoundry/device-sdk-go#422

Closed

lenny added 5 commits December 10, 2019 09:10

Updates from Design Review

8b9862a

- Added that config provider information will be logged - Changed Stem to Basepath in the New Configuration Client Config - Added section for Config endpoint chnages to include the config provider information Signed-off-by: lenny <leonard.goodell@intel.com>

Changed PutConfigurationToml to utConfigurationFromToml in new Config…

ec98a2a

…uration Client interface Signed-off-by: lenny <leonard.goodell@intel.com>

Merge branch 'RefactorRegistry' of https://github.com/lenny-intel/edg…

03a02b1

…ex-go into RefactorRegistry # Conflicts: # adr/Registry Refactoring Design.md

Changed PutConfigurationToml to utConfigurationFromToml in new Config…

b3ff101

…uration Client interface Signed-off-by: lenny <leonard.goodell@intel.com>

lenny-goodell dismissed cloudxxx8’s stale review via b3ff101 December 10, 2019 16:16

michaelestrin mentioned this pull request Dec 16, 2019

standardize consul location options/env vars #1822

Closed

Merge branch 'master' into RefactorRegistry

f86d163

lenny-goodell closed this Jan 22, 2020

lenny-goodell deleted the RefactorRegistry branch January 22, 2020 00:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

design(Refactor Registry): Create a design for refactoring the Registry component #2133

design(Refactor Registry): Create a design for refactoring the Registry component #2133

lenny-goodell commented Nov 13, 2019 •

edited

mkbhanda Nov 15, 2019

lenny-goodell Nov 25, 2019

mkbhanda left a comment

mkbhanda Nov 15, 2019

lenny-goodell Nov 25, 2019

iain-anderson Nov 15, 2019

lenny-goodell Nov 25, 2019

cloudxxx8 Dec 8, 2019

iain-anderson Nov 15, 2019

lenny-goodell Nov 25, 2019

codecov-io commented Nov 15, 2019 •

edited

cloudxxx8 left a comment

cloudxxx8 Dec 8, 2019

lenny-goodell commented Dec 10, 2019

		If no value is provided to the `-cp/-configProvider` option, i.e. just `-cp`, and no environment variable override is specified, the default value of `consul.http://localhost:8500` will be used.

		if `-cp/-configProvider` not used and no environment variable override is specified the local configuration file is used, as is it now.

		if `-cp/-configProvider` not used and no environment variable override is specified the local configuration file is used, as is it now.

		The existing `-r/-registry` command line option will be removed from all services.

design(Refactor Registry): Create a design for refactoring the Registry component #2133

design(Refactor Registry): Create a design for refactoring the Registry component #2133

Conversation

lenny-goodell commented Nov 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkbhanda left a 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

codecov-io commented Nov 15, 2019 • edited

Codecov Report

cloudxxx8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenny-goodell commented Dec 10, 2019

lenny-goodell commented Nov 13, 2019 •

edited

codecov-io commented Nov 15, 2019 •

edited