feat: Added GetDetailedFeatureDecisionUnsafe #266
Conversation
mjc1283
left a comment
mjc1283
left a comment
There was a problem hiding this comment.
LGTM. Small suggestions. Can we add basic tests for this method?
pkg/client/client.go
Outdated
| } | ||
| } | ||
|
|
||
| out, typedError := o.getTypedValue(val, v) |
There was a problem hiding this comment.
Nit: out isn't an ideal variable name IMO. What does it mean?
| if decisionInfo.Enabled { | ||
| if variable, ok := featureDecision.Variation.Variables[v.ID]; ok { | ||
| val = variable.Value | ||
| } |
There was a problem hiding this comment.
If ok is false, let's log an error.
pkg/client/client.go
Outdated
| decisionNotification.Type = notification.AllFeatureVariables | ||
|
|
||
| if err = o.notificationCenter.Send(notification.Decision, *decisionNotification); err != nil { | ||
| o.logger.Warning("Problem with sending notification") |
There was a problem hiding this comment.
Let's log err here so the user can see what the problem was.
pkg/client/client.go
Outdated
|
|
||
| func (o *OptimizelyClient) getTypedValue(value string, variable entities.Variable) (out interface{}, err error) { | ||
| out = value | ||
| switch varType := variable.Type; varType { |
There was a problem hiding this comment.
Why do we need to use a new variable varType? Does this not work?
| switch varType := variable.Type; varType { | |
| switch variable.Type { |
pkg/client/client.go
Outdated
| decisionInfo.VariationKey = featureDecision.Variation.Key | ||
| decisionInfo.ExperimentKey = featureDecision.Experiment.Key |
There was a problem hiding this comment.
I think we should only assign these when featureDecision.Source == decision.FeatureTest. Keys from a rollout experiment should not be exposed.
mikecdavis
left a comment
mikecdavis
left a comment
There was a problem hiding this comment.
Approve pending requests made by @mjc1283
pkg/client/client.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (o *OptimizelyClient) getTypedValue(value string, variable entities.Variable) (out interface{}, err error) { |
There was a problem hiding this comment.
do we need to pass the whole variable ? Seems like only type will be enough. that will also simplify switch statement
mjc1283
left a comment
mjc1283
left a comment
There was a problem hiding this comment.
The tests are pretty verbose, @pawels-optimizely any ideas on how to make them more concise?
Requesting 2 changes related to test setup.
pkg/client/client_test.go
Outdated
| type variable struct { | ||
| key string | ||
| defaultVal string | ||
| varVal string | ||
| varType entities.VariableType | ||
| expected interface{} | ||
| } |
There was a problem hiding this comment.
The same struct type is declared in the previous test. I suggest declaring it only once inside this file.
pkg/client/client_test.go
Outdated
| variables := []variable{ | ||
| {key: "var_str", defaultVal: "default", varVal: "var", varType: entities.String, expected: "var"}, | ||
| {key: "var_bool", defaultVal: "false", varVal: "true", varType: entities.Boolean, expected: true}, | ||
| {key: "var_int", defaultVal: "10", varVal: "20", varType: entities.Integer, expected: 20}, | ||
| {key: "var_double", defaultVal: "1.0", varVal: "2.0", varType: entities.Double, expected: 2.0}, | ||
| {key: "var_json", defaultVal: "{}", varVal: "{\"field1\":12.0, \"field2\": \"some_value\"}", varType: entities.JSON, | ||
| expected: map[string]interface{}{"field1": 12.0, "field2": "some_value"}}, | ||
| } | ||
|
|
||
| mockConfig := new(MockProjectConfig) | ||
| variableMap := make(map[string]entities.Variable) | ||
| varVariableMap := make(map[string]entities.VariationVariable) | ||
|
|
||
| for i, v := range variables { | ||
| id := strconv.Itoa(i) | ||
| varVariableMap[id] = entities.VariationVariable{ | ||
| ID: id, | ||
| Value: v.varVal, | ||
| } | ||
|
|
||
| variableMap[id] = entities.Variable{ | ||
| DefaultValue: v.defaultVal, | ||
| ID: id, | ||
| Key: v.key, | ||
| Type: v.varType, | ||
| } | ||
|
|
||
| mockConfig.On("GetVariableByKey", testFeatureKey, v.key).Return(v.varVal, nil) | ||
| } |
There was a problem hiding this comment.
This logic is repeated in several tests, please factor this out into a helper function.
the client tests with GetFeatureVariable were rewritten some time back using test tables for variable results. We can try to look other tests and see what else all tests share in common. however, if you are referring to DecisionUnsafe tests then these would be gone anyway. So to make tests more concise, you init common things in the setup and then use test tables. |
mjc1283
left a comment
mjc1283
left a comment
There was a problem hiding this comment.
Still requesting changes for 2 inline comments from earlier
Summary
Temporary changes in go-sdk for adding a new method GetDetailedFeatureDecisionUnsafe
This is added to support some agent features. Which will be removed later on.
Testplan