Reorganize config manager packages in agent #1198
Conversation
Signed-off-by: Pavol Loffay <[email protected]>
pavolloffay
requested review from
black-adder,
jpkrohling,
objectiser,
tiffon,
vprithvi and
yurishkuro
as code owners
ghost
assigned 
|
@jpkrohling would you like to have a look? |
Codecov Report
@@ Coverage Diff @@
## master #1198 +/- ##
======================================
Coverage 100% 100%
======================================
Files 159 159
Lines 7136 7136
======================================
Hits 7136 7136
Continue to review full report at Codecov.
|
| @@ -12,14 +12,15 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| package httpserver | |||
| package tchannel | |||
There was a problem hiding this comment.
I'm not a big fan of the new package structure: without looking at the code, I would expect that the packages cmd/agent/app/httpserver/[tchannel|grpc] would contain code that ties something about gRPC or TChannel with the HTTP server that the agent starts, when in reality, all these packages have are specific implementations of the ClientConfigManager interface.
Perhaps the managers deserve their own packages? cmd/agent/app/clientconfigmanager/[tchannel|grpc]? Perhaps they should even be outside of the cmd package.
There was a problem hiding this comment.
The manager interface is used only in httpserver package Therefore it was originally colocated there I am fine scoping it under httpserver.
Perhaps they should even be outside of the cmd package.
This applies also to reporter and probably other subpackages. It would make sense to move them if they could be reused in other cmd. This is specific to the agent only.
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
|
@jpkrohling I have moved the configmanager to a separate package. Is there anything more? |
No, that was it. Looks great :-) |
Resolves #1194
Signed-off-by: Pavol Loffay [email protected]