Reuse injectproxy package#66
Conversation
Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
bwplotka
left a comment
There was a problem hiding this comment.
Amazing, small nits, otherwise LGTM! (:
| return nil | ||
| } | ||
|
|
||
| func (ms Enforcer) enforceMatchers(targets []*labels.Matcher) []*labels.Matcher { |
There was a problem hiding this comment.
Do you mind adding quick commentary what this public method does (Public methods should be commented always with comment started as // EnforceMatchers ...
| ) | ||
|
|
||
| type Enforcer struct { | ||
| labelMatchers map[string]*labels.Matcher |
There was a problem hiding this comment.
Why we need access to this? It looks like code smell, as it leaks the enforcer internal logic. Aren't its methods enough? Are you missing some functionality that could be added as method?
There was a problem hiding this comment.
We need to access this because enforceMatchers is a method of enforcer and to access it first we need to create an enforcer(in our use case in Thanos this enforcer contains the information about the tenant label name we want to replace and tenant access a user has as specified in the header)
There was a problem hiding this comment.
yea, but this method has access to this because it's a method on Enforcer itself, no?
There was a problem hiding this comment.
hmm, I think we can use NewEnforcer to create a new enforcer.
Let me try it.
There was a problem hiding this comment.
Well. I think we even have to! That's why we do constructors in Go (:
There was a problem hiding this comment.
ahh just refreshed those concepts thanks! definitely make sense.
Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
| } | ||
|
|
||
| func (ms Enforcer) enforceMatchers(targets []*labels.Matcher) []*labels.Matcher { | ||
| // EnforceMatchers replaces similar matcher present in |
There was a problem hiding this comment.
I would be specific what "similar" means, but it's good enough (:
There was a problem hiding this comment.
will keep that in mind.
Signed-off-by: Abhishek357 abhisheksinghchauhan357@gmail.com
We would like to reuse the injectproxy package in thanos. Related to PR