Move internal/x/settings to internal/settings #77
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #77 +/- ##
==========================================
- Coverage 67.88% 67.63% -0.25%
==========================================
Files 68 68
Lines 3783 3726 -57
==========================================
- Hits 2568 2520 -48
+ Misses 880 874 -6
+ Partials 335 332 -3
Continue to review full report at Codecov.
|
internal/settings/config_provider.go
Outdated
| filePathToConfig map[string]Config | ||
| dirPathToFilePath map[string]string | ||
| dirPathToExcludePrefixes map[string][]string | ||
| lock sync.RWMutex |
There was a problem hiding this comment.
There is a lot of logic written around making the configProvider concurrent-safe. Is this really necessary? In the places that this might be called concurrently, such as in calls in internal/x/file, can we instead synchronously run the commands?
I'm not sure how much of a performance gain we expect to see in this domain, so it might be worth removing it for now.
What do you think? Is this actually completely necessary / am I missing something?
There was a problem hiding this comment.
Things like caches (which is what this is locking) should always be thread-safe, IMO, I don't see it as an optional thing. I don't want our designs to have to rely on something being not thread-safe, or worse, someone using this in a deep dependency in a threaded manner without realizing this is not thread-safe. It's defensive programming if anything, but a cache should generally always be thread-safe unless there's a compelling reason not to be IMO.
|
I ripped out the caching in e85e696 - it actually doesn't clean it up that much, but it doesn't seem to make any difference in time when I run it against googleapis. I'll play with it more but do you like this more? |
internal/settings/settings.go
Outdated
| // The prefixes to exclude. | ||
| // Expected to be absolute paths. | ||
| // Expected to be unique. | ||
| ExcludePrefixes []string |
There was a problem hiding this comment.
Thoughts on renaming this Blacklist?
internal/settings/settings.go
Outdated
| // It is meant to be set by a YAML or JSON config file, or flags. | ||
| type ExternalConfig struct { | ||
| Excludes []string `json:"excludes,omitempty" yaml:"excludes,omitempty"` | ||
| NoDefaultExcludes bool `json:"no_default_excludes,omitempty" yaml:"no_default_excludes,omitempty"` |
There was a problem hiding this comment.
Re: #49 -
Should we remove this?
internal/settings/config_provider.go
Outdated
| includePath = filepath.Join(dirPath, includePath) | ||
| } | ||
| includePath = filepath.Clean(includePath) | ||
| //if includePath != dirPath { |
|
I prefer the implementation without the cache, especially because I don't know how often we need to retrieve a configuration for the same file path (rather than simply accessing the configuration that was already retrieved once). It does, however, make it so that we will unmarshal the yaml every time the file path is requested. I'm not saying that we should do this here, but we should consider the |
| @@ -28,27 +28,19 @@ import ( | |||
| "sort" | |||
| "strconv" | |||
| "strings" | |||
| "sync" | |||
|
|
|||
| "github.com/uber/prototool/internal/strs" | |||
| "go.uber.org/zap" | |||
| "gopkg.in/yaml.v2" | |||
| ) | |||
|
|
|||
| type configProvider struct { | |||
There was a problem hiding this comment.
nit: Now that the cache is removed, it looks like we don't actually need this type. The logger is never used and all of the configProvider methods don't access any of the type's attributes. In other words, all of the methods could be package-level functions now.
Removing this later might become more relevant depending on the direction we take #93. If we only ever need one prototool.yaml, simple package-level functions might make more sense since we only need the single configuration struct.
There was a problem hiding this comment.
I still want to keep this abstraction for now, but we can revisit after #93.
No description provided.