Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

cluster and identity_cluster tables #24

Merged
merged 6 commits into from
Nov 7, 2018

Conversation

alexeykazakov
Copy link
Contributor

Fixes #9

@alexeykazakov alexeykazakov changed the title cluster and cluster cluster and identity_cluster tables Nov 6, 2018
@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #24 into master will increase coverage by 12.92%.
The diff coverage is 95.83%.

Impacted file tree graph

@@           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
Impacted Files Coverage Δ
controller/clusters.go 100% <100%> (ø) ⬆️
configuration/configuration.go 76.68% <100%> (ø) ⬆️
migration/migration.go 80% <88.88%> (+39.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a7e440...9127aea. Read the comment docs.

Copy link
Contributor

@xcoulon xcoulon left a 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.

application/transaction/transaction_test.go Outdated Show resolved Hide resolved
application/transaction/transaction_test.go Outdated Show resolved Hide resolved
migration/migration_blackbox_test.go Outdated Show resolved Hide resolved
@@ -4,7 +4,6 @@ import (
"context"
Copy link
Contributor

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 ?

Copy link
Contributor Author

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 (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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.

@alexeykazakov alexeykazakov merged commit bddfb4e into fabric8-services:master Nov 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants