Refactor code into separate packages. #752
Conversation
fecd54c
to
3ea403d
Compare
Move existing files into a few separate packages. This should improve testability and make future refactorings easier.
3ea403d
to
8bf3b23
Compare
| @@ -1,4 +1,4 @@ | |||
| package exporter | |||
| package services | |||
There was a problem hiding this comment.
This file feels a little out of place here since it's not helping define supported services like services.go. TaggedResource and FilterThroughTags is used by services but I I'm not sure the services pkg owns the definition of a TaggedResource. I think it might fit better with the job pkg, WDYT?
| return nil | ||
| } | ||
|
|
||
| func (c *ScrapeConf) Load(file *string, validSvc func(string) bool) error { |
There was a problem hiding this comment.
This refactor stood out, it feels a bit odd to allow a consumer to provide a func which validates the services. I think I would expect a defined way to do that as the default behavior of loading the config. What are your thoughts here?
There was a problem hiding this comment.
Totally agree. I've resorted to this approach to avoid a dependency loop: almost all other pkgs depend on config, including services, so couldn't make config depend on services.
Might move services.go into config pkg... a bit extreme but might make sense on a level. What do you suggest?
There was a problem hiding this comment.
Ah yeah I didn't think about the depenency loop 😢 . I think moving services.go to the config pkg sounds like a good plan for now.
There was a problem hiding this comment.
Second thought, related to the comment on aws_tags.go above. Moving services.go to pkg/config make the latter depend on pkg/services (or whatever other package where aws_tags.go is placed). Again a loop.
A possible alternative is a code refactoring where services.go is split in something like: all the const stuff (names, aliases, etc) is moved to a struct of some sort in pkg/config, while the {Filter,Resource}Func and everything else that depends on aws_tags.go definitions is kept in services.go. (plus some additional changes here and there to deal with the fact SupportedServices is being split)
There was a problem hiding this comment.
@kgeckhart wdyt if this is merged as-is and I give a try to a code refactor just after it? (before cutting a new release anyway)
There was a problem hiding this comment.
👍 makes sense to let this clear and adjust further later. This is already a great step in the right direction.
Extract out supported services list into the config package, and leave filtering funcs in the services package. Followup of #752
Extract out supported services list into the config package, and leave filtering funcs in the services package. Followup of #752
Move existing files into a few separate packages. This should improve testability and make future refactorings easier.