Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLOUDTRUST-2375] Add middleware to enable feature according to reaml conf. #41

Merged
merged 1 commit into from
Mar 4, 2020

Conversation

fperot74
Copy link
Contributor

@fperot74 fperot74 commented Mar 2, 2020

No description provided.

@coveralls
Copy link

coveralls commented Mar 2, 2020

Coverage Status

Coverage increased (+1.3%) to 89.416% when pulling 74a6646 on ct-2375 into b9f14ad on master.

Copy link
Contributor

@harture harture left a comment

Choose a reason for hiding this comment

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

Seems the tests fails, and an argument is unused.
Except those 2 small points, fine for me

middleware/middleware.go Outdated Show resolved Hide resolved
@@ -37,6 +38,50 @@ func MakeEndpointInstrumentingMW(m metrics.Metrics, histoName string) cs.Middlew
}
}

// IDRetriever is an interface to get an ID using an object's name
type IDRetriever interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this abstraction ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we need this abstraction ?

Because implementation is done in keycloak-client and keycloak-client refers to common-service... So we should not add a reference to keycloak-client in this library

@fperot74 fperot74 merged commit f4ba60b into master Mar 4, 2020
@fperot74 fperot74 deleted the ct-2375 branch March 10, 2020 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants