Refactor config package and add functionality #44
Conversation
samcoe
force-pushed
the
config-integration
branch
6 times, most recently
from
7c2c2f1
to
80af8a5
Compare
samcoe
force-pushed
the
config-integration
branch
from
80af8a5
to
9041602
Compare
pkg/auth/auth.go
Outdated
| hostsKey = "hosts" | ||
| ) | ||
|
|
||
| func TokenForHost(cfg *config.Config, host string) (string, string) { |
There was a problem hiding this comment.
Should these top-level functions take cfg as argument, or instead just access the config.Config instance internally as a singleton?
There was a problem hiding this comment.
Yeah I think I agree, these functions feel like they should know how to access the config themselves, I can't think of a scenario where we would want the user to pass in their own config.Config instance. My only worry is that config.Config becomes an implicit dependency of these functions rather than an explicit one.
pkg/config/config.go
Outdated
| entries *yamlmap.Map | ||
| } | ||
|
|
||
| func Get(c *Config, keys []string) (string, error) { |
There was a problem hiding this comment.
Now that config.Config is just a struct and not some interface anymore, what are the tradeoffs of having config.Get/Set/Remove etc. be methods on the top level instead of on the struct itself?
There was a problem hiding this comment.
That is a good question. I don't know if I can think of a pro/con for either way. Perhaps having them be methods on the struct is more idiomatic? I don't have strong opinions on either direction.
There was a problem hiding this comment.
After thinking about this more I couldn't come up with a good reason not to make these methods so I made the change.
samcoe
force-pushed
the
config-integration
branch
from
6e036a7
to
ae1157f
Compare
|
@mislav This is ready for re-review when you get a chance 🙇 |
This PR introduces a bunch of functionality necessary for
ghto utilizego-ghfor config handling.Things changed:
configmaptoyamlmapconfigpackage with functions for manipulating configuration files. Only business logic that is encapsulated is around reading and writing of configuration files.authpackage with functions related to auth tokens and hosts. This functionality is not new, it just used to live in the internalconfigpackage.I added inline comments and questions for design decisions.
Note: This PR is not as long as it seems, lots of code was just shuffled around.
cc cli/cli#5560