Config impl #567

shawn-hurley · 2017-11-21T13:54:42Z

Describe what this PR does and why we need it:
Implementation of the config proposal
Changes proposed in this pull request

To many to list. Redoing the entire config management

shawn-hurley · 2017-11-21T14:13:07Z

@eriknelson @jmrodri @rthallisey @djzager Some testing help would be nice, this touches so much stuff, that I will forget something during my testing. Things that I am most worried about

Does this seem to make it actually more complicated to add config values?
Make sure there are no inconsistencies in the initialization of config files. I would like a create one way of doing it and stick with that. Here is why I am thinking it should be the responsibility of the package to create its config.

The package knows about its changes and can also add validation/defaults at that point.
We don't want app to interact with any other package then log and config, to facilitate the logging changes in its proposal.

The secrets config section might not work.

coveralls · 2017-11-21T14:42:18Z

Changes Unknown when pulling 10f60ec on shawn-hurley:config-impl into ** on openshift:master**.

coveralls · 2017-11-21T17:07:18Z

Changes Unknown when pulling 3694e65 on shawn-hurley:config-impl into ** on openshift:master**.

jmrodri · 2017-11-22T02:29:42Z

pkg/apb/svc_acct.go

@@ -124,7 +124,7 @@ func (s *ServiceAccountManager) CreateApbSandbox(

 func (s *ServiceAccountManager) createResources(rFilePath string, namespace string) error {
 	s.log.Debug("Creating resources from file at path: %s", rFilePath)
-	output, err := runtime.RunCommand("oc", "create", "-f", rFilePath)
+	output, err := runtime.RunCommand("oc", "create", "--namespace", namespace, "-f", rFilePath)


is this namespace meant to be here or is it from another commit?

Another commit, I can remove.

jmrodri · 2017-11-22T02:33:19Z

pkg/apb/svc_acct.go

@@ -221,7 +221,7 @@ func (s *ServiceAccountManager) createFile(handle string) (string, error) {
 }

 // DestroyApbSandbox - Destroys the apb sandbox
-func (s *ServiceAccountManager) DestroyApbSandbox(executionContext ExecutionContext, clusterConfig ClusterConfig) {
+func (s *ServiceAccountManager) DestroyApbSandbox(executionContext ExecutionContext) {


I like the removal of the clusterConfig ClusterConfig parameters.

jmrodri · 2017-11-22T03:12:43Z

pkg/app/app.go

-		CertFile: config.Broker.SSLCert,
-		KeyFile:  config.Broker.SSLCertKey,
+		CertFile: config.GetString("broker.ssl_cert"),
+		KeyFile:  config.GetString("broker.ssl_cert_key"),


So config.Broker.SSLCert gave us the compile time benefit. But it was also the source of what made adding stuff to the config painful. I would have to go update the struct vs just adding values to the config. Most projects I've worked with I've used the GetString type methods.

jmrodri · 2017-11-22T03:15:02Z

pkg/app/app.go

@@ -199,11 +201,11 @@ func CreateApp() App {
 	}

 	app.log.Debug("Connecting Registry")
-	for _, r := range app.config.Registry {
-		reg, err := registries.NewRegistry(r, app.log.Logger)
+	for name := range app.config.GetSubConfig("registry").ToMap() {


The GetSubConfig is also acceptable.

coveralls · 2017-11-27T16:30:27Z

Changes Unknown when pulling f2ec97f on shawn-hurley:config-impl into ** on openshift:master**.

coveralls · 2017-11-27T16:31:47Z

Changes Unknown when pulling f2ec97f on shawn-hurley:config-impl into ** on openshift:master**.

jmrodri · 2017-11-27T22:00:43Z

pkg/registries/registry.go

+		WhiteList:  con.GetSliceOfStrings("white_list"),
+		BlackList:  con.GetSliceOfStrings("black_list"),
+		AuthType:   con.GetString("auth_type"),
+		AuthName:   con.GetString("auth_name"),


Is this the thing you mentioned to me where we can have the config struct which gives me compile time validation while using the config?

@jmrodri yes, another example is the broker config in the AnsibleBroker struct.

I think if we move to an Interface & Struct to the APB package, then this is a pattern that we can follow there.

jmrodri

CONDACK pending answers to my questions.

rthallisey · 2017-11-28T16:14:00Z

pkg/apb/secrets.go

@@ -103,27 +104,29 @@ func match(spec *Spec, rule AssociationRule) bool {

 // InitializeSecretsCache - Generates AssociationRules from config and
 // initializes the global secrets cache
-func InitializeSecretsCache(config []SecretsConfig, log *logging.Logger) {
+func InitializeSecretsCache(con *config.Config, log *logging.Logger) {


how about conf instead of con

rthallisey · 2017-11-28T16:44:18Z

pkg/config/config.go

+var (
+	log = logging.MustGetLogger("config")
+)
+


There might be an easier way to share the config data.

// pkg/config/broker.go client.NewConfig(...) // pkg/config/config.go var configFile map[string]interface{} func NewConfig(conf map[string]interface{}) error { ... configFile = conf return nil } func GetConfigValue(value string) (string, error) { .... return configFile[value], nil }

Then every pkg can have access to the var through the GetConfigValue function. If there's nested config values we could also add a GetNestedConfigValue.

config.GetConfigValue(configValue)

coveralls · 2017-11-28T17:37:14Z

Changes Unknown when pulling 6540b2c on shawn-hurley:config-impl into ** on openshift:master**.

coveralls · 2017-11-29T14:26:11Z

Changes Unknown when pulling 81c30a6 on shawn-hurley:config-impl into ** on openshift:master**.

coveralls · 2017-11-29T21:40:59Z

Changes Unknown when pulling af9d7e0 on shawn-hurley:config-impl into ** on openshift:master**.

coveralls · 2017-11-29T22:18:47Z

Changes Unknown when pulling 9bb233d on shawn-hurley:config-impl into ** on openshift:master**.

eriknelson

CONDACK

Few questions just curious about, otherwise I think this is a big improvement. Especially happy to get rid of config references in every fn sig 👍

One thought that comes to mind is that it will be very easy to fat finger a config value lookup string. It's maybe less of a problem with some types, but often we may take two very different behavioral branches depending on a config value. I can see us taking a falsy branch here unintentionally with just a debug log to indicate what actually happened.

Maybe we want to kick those up to warning level logs? Not sure I have a better option to offer here, but kind of curious what other people think.

Overall, nice job @shawn-hurley !

eriknelson · 2017-12-01T18:38:13Z

pkg/broker/broker.go

 	}
-
-	err := broker.Login()


Why are we able to get rid of all of this? Looking this over again, I'm not a fan of this being in a New* method, so I'm a proponent of pulling it out, but I want to make sure this isn't a regression.

We are able to get rid of this because the initialization of the clients does the things that we need. I think this code has not been needed for a long time. I can add it back if people don't want it removed in this PR.

Might be better to add it back and remove in an explicit PR?

@eriknelson I just realized while adding it back why I removed it. I removed the clusterConfig from the broker object causing this to be a problem where it is with this PR.

I did not add it back because this will just initialize the OC client, while everything should no longer be using the local oc client.

I've been commenting this out locally for a few days now because it messes up the local k8s deployment. I don't think we need it anymore. Removing it will also be one less thing we need to add to the runtime pkg.

Should we also be removing the Config Values as well (Host and BearerTokenFile)

Do we know of folks wanting to use their own bearer tokens instead of using auth that k8s provides? To me, I think it would be safe to remove it. We could always add it back in later if that is a valid use case.

eriknelson · 2017-12-01T18:43:55Z

pkg/clients/etcd.go

 }

 // GetEtcdVersion - Connects to ETCD cluster and retrieves server/version info
 func GetEtcdVersion(ec EtcdConfig) (string, string, error) {
 	// The next etcd release (1.4) will have client.GetVersion()
 	// We'll use this to test our etcd connection for now
-	etcdURL := fmt.Sprintf("http://%s:%s/version", ec.EtcdHost, ec.EtcdPort)
+	etcdURL := fmt.Sprintf("http://%s:%v/version", ec.EtcdHost, ec.EtcdPort)


For my own edification, what's the purpose of this change?

EtcdPort was changed to an int, %s assumes a string, %v is just the value of the thing so works for everything. https://golang.org/pkg/fmt/ if you want to see all the options that fmt gives you.

coveralls · 2017-12-04T20:54:40Z

Changes Unknown when pulling 8c27f14 on shawn-hurley:config-impl into ** on openshift:master**.

coveralls · 2017-12-04T21:56:37Z

Changes Unknown when pulling 31e9156 on shawn-hurley:config-impl into ** on openshift:master**.

coveralls · 2017-12-05T21:11:52Z

Changes Unknown when pulling 08dc5a8 on shawn-hurley:config-impl into ** on openshift:master**.

* adding configuration package * changing everyting for configuration changes * fixing test handler. * rebase changes. * fixing tests * fixing variable name from review comments * fixing logging debug statements that were left over. * removing Host, CAFILE, and BearerToken as they are no longer used. * fixing rebase of app.go

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 21, 2017

shawn-hurley requested review from jmrodri, eriknelson, djzager and rthallisey November 21, 2017 14:01

shawn-hurley added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. feature needs-review tech-debt labels Nov 21, 2017

shawn-hurley force-pushed the config-impl branch from 2c87ec0 to 10f60ec Compare November 21, 2017 14:03

jmrodri reviewed Nov 22, 2017

View reviewed changes

shawn-hurley force-pushed the config-impl branch from 3694e65 to da4aaaa Compare November 27, 2017 16:01

shawn-hurley removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 27, 2017

jmrodri reviewed Nov 27, 2017

View reviewed changes

jmrodri approved these changes Nov 27, 2017

View reviewed changes

rthallisey reviewed Nov 28, 2017

View reviewed changes

shawn-hurley force-pushed the config-impl branch from 6540b2c to 81c30a6 Compare November 29, 2017 13:48

shawn-hurley force-pushed the config-impl branch from 81c30a6 to af9d7e0 Compare November 29, 2017 21:04

eriknelson reviewed Dec 1, 2017

View reviewed changes

eriknelson approved these changes Dec 1, 2017

View reviewed changes

shawn-hurley force-pushed the config-impl branch from 9bb233d to 8c27f14 Compare December 4, 2017 20:15

shawn-hurley added 9 commits December 5, 2017 15:05

adding configuration package

74b6006

changing everyting for configuration changes

0860012

fixing test handler.

15591bc

rebase changes.

6e7c347

fixing tests

7284b01

fixing variable name from review comments

fb5dc23

fixing logging debug statements that were left over.

836b9cf

removing Host, CAFILE, and BearerToken as they are no longer used.

a83bbf8

fixing rebase of app.go

08dc5a8

shawn-hurley force-pushed the config-impl branch from 31e9156 to 08dc5a8 Compare December 5, 2017 20:32

rthallisey approved these changes Dec 6, 2017

View reviewed changes

rthallisey merged commit 14e5177 into openshift:master Dec 6, 2017

Config impl #567

Config impl #567

shawn-hurley commented Nov 21, 2017

shawn-hurley commented Nov 21, 2017

coveralls commented Nov 21, 2017

coveralls commented Nov 21, 2017 •

edited

jmrodri Nov 22, 2017

shawn-hurley Nov 27, 2017

jmrodri Nov 22, 2017

jmrodri Nov 22, 2017

jmrodri Nov 22, 2017

coveralls commented Nov 27, 2017

coveralls commented Nov 27, 2017 •

edited

jmrodri Nov 27, 2017

shawn-hurley Nov 28, 2017

jmrodri left a comment

rthallisey Nov 28, 2017

rthallisey Nov 28, 2017

coveralls commented Nov 28, 2017

coveralls commented Nov 29, 2017

coveralls commented Nov 29, 2017

coveralls commented Nov 29, 2017

eriknelson left a comment

eriknelson Dec 1, 2017

shawn-hurley Dec 1, 2017 •

edited

eriknelson Dec 1, 2017

shawn-hurley Dec 4, 2017

rthallisey Dec 4, 2017

shawn-hurley Dec 4, 2017

rthallisey Dec 4, 2017

eriknelson Dec 1, 2017

shawn-hurley Dec 1, 2017

coveralls commented Dec 4, 2017

coveralls commented Dec 4, 2017

coveralls commented Dec 5, 2017

Config impl #567

Config impl #567

Conversation

shawn-hurley commented Nov 21, 2017

shawn-hurley commented Nov 21, 2017

coveralls commented Nov 21, 2017

coveralls commented Nov 21, 2017 • edited

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

coveralls commented Nov 27, 2017

coveralls commented Nov 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 28, 2017

coveralls commented Nov 29, 2017

coveralls commented Nov 29, 2017

coveralls commented Nov 29, 2017

eriknelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley Dec 1, 2017 • edited

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

coveralls commented Dec 4, 2017

coveralls commented Dec 4, 2017

coveralls commented Dec 5, 2017

coveralls commented Nov 21, 2017 •

edited

coveralls commented Nov 27, 2017 •

edited

shawn-hurley Dec 1, 2017 •

edited