Skip to content

feat(integrations): Added support for integrations in datafile and evaluation. #350

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Nov 7, 2022
Merged

feat(integrations): Added support for integrations in datafile and evaluation. #350

merged 12 commits into from
Nov 7, 2022

Conversation

yasirfolio3
Copy link
Contributor

@yasirfolio3 yasirfolio3 commented Oct 19, 2022 •
edited
Loading

Summary

  • Parse new integrations section of the datafile
  • Evaluate() using UserContext

Test plan

  • All new and previous tests should pass

Issues

FSSDK-8518

@yasirfolio3 yasirfolio3 requested a review from a team as a code owner October 19, 2022 09:00
msohailhussain
msohailhussain suggested changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

partial review. need to check matcher conditions code.

examples/main.go
@@ -7,7 +7,7 @@ import (
"fmt"
"time"

"github.com/optimizely/go-sdk"
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was forced by linter since we are using optimizely keyword to access the package in the sample code.

pkg/client/optimizely_user_context.go
@@ -160,3 +189,11 @@ func copyUserAttributes(attributes map[string]interface{}) (attributesCopy map[s
}
return attributesCopy
}

func copyQualifiedSegments(qualifiedSegments []string) (qualifiedSegmentsCopy []string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

need to check offline.

pkg/client/optimizely_user_context_test.go Outdated
@@ -17,7 +17,7 @@
package client

import (
"io/ioutil"
"os"
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested by linter as "io/ioutil is now depreceated in go 1.16 onwards.

pkg/config/datafileprojectconfig/config.go
var hostForODP, publicKeyForODP string
if len(datafile.Integrations) > 0 {
integration := datafile.Integrations[0]
if integration.Key == "odp" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems weird, can we 'iterate integrationsand checkodp, assign it and get out of loop`

pkg/config/datafileprojectconfig/config.go Outdated
attributeMap, attributeKeyToIDMap := mappers.MapAttributes(datafile.Attributes)
allExperiments := mappers.MergeExperiments(datafile.Experiments, datafile.Groups)
groupMap, experimentGroupMap := mappers.MapGroups(datafile.Groups)
experimentIDMap, experimentKeyMap := mappers.MapExperiments(allExperiments, experimentGroupMap)

rollouts, rolloutMap := mappers.MapRollouts(datafile.Rollouts)
integrations := mappers.MapIntegrations(datafile.Integrations)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this map? integrationsMap?

pkg/config/datafileprojectconfig/mappers/integration.go Outdated
)

// MapIntegrations maps the raw datafile integration entities to SDK Integration entities
func MapIntegrations(integrations []datafileEntities.Integration) (integrationsList []entities.Integration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a map, simple list either return map along with list or no need of this method.

pkg/decision/evaluator/condition.go
reasons := decide.NewDecisionReasons(options)
if condition.Type != customAttributeType {
isValid := false
for _, validType := range validTypes {
Copy link
Contributor

Choose a reason for hiding this comment

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

why nchanged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have two valid types that we support now, this can be seen here: https://github.com/optimizely/go-sdk/pull/350/files#diff-4b69e47626e15aedb2e8d1bb3b34fdb04fd57af036e7c00f9f968a5490d24750R35

Base automatically changed from yasir/lru-cache to master October 20, 2022 05:10
@yasirfolio3 yasirfolio3 reopened this Oct 20, 2022
jaeopt
jaeopt requested changes Oct 20, 2022
View reviewed changes
Copy link

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Looks great. A few changes suggested.

pkg/client/optimizely_user_context.go Outdated
Comment on lines 102 to 104
if qualifiedSegments == nil {
qualifiedSegments = []string{}
}
Copy link

Choose a reason for hiding this comment

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

qualifiedSegments are nullable. When null, it indicates non-determined (fetchSegments are not completed yet). Empty segments is valid one (fetchSegments returns an empty list).

pkg/client/optimizely_user_context.go Outdated
@@ -46,6 +47,7 @@ func newOptimizelyUserContext(optimizely *OptimizelyClient, userID string, attri
return OptimizelyUserContext{
UserID: userID,
Attributes: attributesCopy,
qualifiedSegments: []string{},
Copy link

Choose a reason for hiding this comment

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

Why passing empty array instead of a copy of current qualifiedSegments?

pkg/config/datafileprojectconfig/config.go
@@ -33,6 +33,8 @@ var datafileVersions = map[string]struct{}{
// DatafileProjectConfig is a project config backed by a datafile
type DatafileProjectConfig struct {
datafile string
hostForODP string
publicKeyForODP string
Copy link

Choose a reason for hiding this comment

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

I see that a logic to get all segments used in datafile audiences is missing. We need it later when we call fetchSegments. See https://github.com/optimizely/swift-sdk/blob/ec83c87d53b17ea58f8c73eb581b27fa52b6305c/Sources/Data%20Model/ProjectConfig.swift#L151

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 was originally holding it off for the next PR but have now implemented it in this one. Please have a look at the new changes.

@yasirfolio3
1. Audience segments added.
e9ad97b 
2. Suggested changes made.
jaeopt
jaeopt approved these changes Oct 24, 2022
View reviewed changes
Copy link

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

msohailhussain
msohailhussain approved these changes Nov 7, 2022
View reviewed changes
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm

@msohailhussain msohailhussain merged commit f6b91bc into master Nov 7, 2022
@msohailhussain msohailhussain deleted the yasir/datafile-parsing-audience-evaluation branch November 7, 2022 06:22
msohailhussain
msohailhussain reviewed Nov 7, 2022
View reviewed changes
pkg/config/datafileprojectconfig/mappers/audience.go
if err == nil {
audience.ConditionTree = conditionTree
}

for _, s := range fSegments {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msohailhussain msohailhussain msohailhussain approved these changes

@jaeopt jaeopt jaeopt approved these changes

Assignees

@yasirfolio3 yasirfolio3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yasirfolio3 @msohailhussain @jaeopt