Allow environment variable to specify which user-provided-service to target

[aeijdenberg]

No description provided.

[jcscottiii]

@aeijdenberg This is nice! I was looking into adding something similar. Could you change it so that you can set multiple UPS values inside this env var? The use case is that for best practices, you should separate the values into isolated UPS. For example you could have a dashboard-cf-ups, dashboard-smtp-ups. Then you can update one accordingly without worrying about the other values.

[aeijdenberg]

@jcscottiii - thanks for the suggestion, done.

I think we're likely to use this same approach in a number of apps that we have - amongst other things, it makes our CI a bit easier.

To make it easier to re-use across projects, I'll plan to take the env var lookup code that we contributed and put it into a common Go library under our public govau GitHub org. Would you like me to send a PR to change cg-dashboard to use that and vendor it in or would you prefer to keep that code embedded in this repo?

[aeijdenberg]

@jcscottiii - I went ahead and pre-emptively did that, so that you could see what the change would look like.

[aeijdenberg]

rebased to fix merge conflict

[aeijdenberg]

(by the way, I'm also quite happy to move that to new repo in the 18F org rather than the govau one if preferred - my main goal is to separate out the common library code so that we could import easily into other apps, we want to use a common pattern from a number of CF apps)

[jcscottiii]

@aeijdenberg first, apologies for the late review.

i like the idea of having it be separate out into a helper library.
Where it lives (the github org), doesn't matter to me. I think once it's mature, we could possibly put it in the cloudfoundry-community org.

Now, I have a problem with a library that creates/uses third party environment variables (i.e. UPS_PATH) https://github.com/govau/cf-common/blob/master/env_vars.go#L94

I have been bitten by other Go projects where I used a library that used another library that expected its own env vars and it didn't become evident until the first run and we had to dig through the code.

I have a few alternatives:

1. Make NewDefaultEnvLookup() accept a variable with the env var string to use. (You pass in the string to use in os.getenv)

• Con: you are still tightly coupled to the delimiter that you need set by the library.

2. Make NewDefaultEnvLookup() accept a variable with the value of the env var (it'll probably be a string slice)

• Pro: the library user parses the env var and delimiter for you
• Con: the pro is also a con because it's extra work.

3. Use the options pattern and you can get rid of all of your different constructors.
   More about it https://halls-of-valhalla.org/beta/articles/functional-options-pattern-in-go,54/
   I imagine it going something like this

func startApp(port string, env *cfenv.App) {
    envVars = cfcommon.NewEnvLookup(WithMultipleUPS("UPS_PATH", ":"), Or(WithUPS(cfUserProvidedService)))
    //envVars = envVars.WithMultipleUPS("UPS_PATH", ":").Or(WithUPS(cfUserProvidedService))
    // the regular stuff from before
    app, settings, err := controllers.InitApp(envVars, env)
}

that would replace all the logic here https://github.com/18F/cg-dashboard/pull/1258/files#diff-34c6b408d72845d076d47126c29948d1R54

then inside the library

func NewEnvLookup(app cfenv.App, options ...func(*EnvVars)) {
	e := &EnvVars{path:  []EnvLookup{os.LookupEnv}}

	for _, option := range options {
		option(app, e)
	}
}

func WithMultipleUPS(envKey string, delimiter string) func(*EnvVars) {
	return func(app cfenv.App, e *EnvVars) {
		for _, name := range strings.Split(os.Getenv(envKey), delimiter) {
			e.path = append(e.path, NewEnvLookupFromCFAppNamedService(app, name))
		}
	}
}

func WithUPS(upsName string) func(*EnvVars) {
	return func(app cfenv.App, e *EnvVars) {
		e.path = append(e.path, NewEnvLookupFromCFAppNamedService(app, upsName))
	}
}

func Or(opt func(app cfenv.App, e *EnvVars)) func(app cfenv.App, e *EnvVars) {
	return func(app cfenv.App, e *EnvVars) {
		if len(e.path) == 1 && e.path[0] == os.LookupEnv {
			// we detected that it's the default stuff still, let's apply our new option
			opt(e)
		}
		// else, we already have options, so we don't need to apply this.
	}
}

i'm not sure this completely compiles but here's an example here I made that's a shorten example: https://play.golang.org/p/lr0ELM4PeR

Also, before using the cfcommon library, would need to have some unit tests over there.

[jcscottiii]

see feedback in comment

[jcscottiii]

@aeijdenberg alternatively, in my comment above you can rename Or to IfNotDefinedUse that's probably more clear

[jonathaningram]

@jcscottiii I just wanted to give you an update and let you know I'm working on some refactoring of cf-common and also adding some tests for this PR.

cc @aeijdenberg

[jonathaningram]

@jcscottiii could you please take a look now? See my comments/rationale on govau/cf-common here: govau/cf-common#1 (comment). Some comments are very slightly out-of-date as we made some changes after I wrote that, but for the most part they are current.

Thanks! Jon.

cc @aeijdenberg

[jcscottiii]

just some minor comments @jonathaningram

[jcscottiii]

this doesn't need to be a mustbool

[jonathaningram]

The way that we've built MustBool is that you don't actually have to have the env var set. If you don't set it, it will just default to false. This is similar to how the flags package works. However, if you do set it, it must be a valid boolean otherwise you'll get a panic. So, really, the "must bool" is saying "if you set a value for this environment variable it MUST be a valid parsable boolean".

In so many words, here's the doc comment from MustBool:

MustBool gets the boolean environment variable with the given name, if set.
If not set, false is returned. This matches behavior one would often see with
command line boolean arguments: if you don't set it, it defaults to false.
It will panic with an error if it is not a valid boolean.

cc @aeijdenberg

[jcscottiii]

you don't need to re-grab the cfenv.Current() since we already pass it in. just check if it's nil. don't use a switch-case in this instance.

if upsNames != "" && app != nil {
  // create UPS Opts
} else {
  // create default opts
}

also, move the logic for each case into functions. makes it easier to read.

[jonathaningram]

@jcscottiii code in server.go has been refactored into 2 functions. Ready for review.

[jonathaningram]

Cool thanks!

[jcscottiii]

@jonathaningram thanks for this!!!