-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
========================================
+ Coverage 67.07% 80% +12.92%
========================================
Files 12 6 -6
Lines 814 480 -334
========================================
- Hits 546 384 -162
+ Misses 231 79 -152
+ Partials 37 17 -20
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, and it's always great to see more common
pkgs being consumed 🎉
I just provided you with a few minor comments that could be addressed later.
@@ -4,7 +4,6 @@ import ( | |||
"context" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's already a sentry
pkg in https://github.com/fabric8-services/fabric8-common/tree/master/sentry, so what about removing this local pkg instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cluster sentry thing provides userInfo function implementation (and tests for that). The common sentry doesn't have this function. Main reason for that is Token Manager loaded from context. We should implement the default userInfo function in common which will use the token manager from common. But clients still have to be able to use their own userinfo function in case they use their own token manager. Like Auth service.
@@ -2,10 +2,8 @@ package suite | |||
|
|||
import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about removing this file in favor of https://github.com/fabric8-services/fabric8-cluster/tree/master/test/suite ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you mean github.com/fabric8-services/fabric8-common/test/suite ?
This suite extends the common suite. Just adds cluster configuration initialization on top to that.
cluster
andidentity_cluster
tables as the first part of Endpoint to get clusters available to particular user #15Fixes #9