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

Revert "feat: add FeatureEnabled to Variant" #165

Merged
merged 1 commit into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion api/feature.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ func (vc VariantCollection) GetVariant(ctx *context.Context) *Variant {
variant = &v.Variant
}
variant.Enabled = true
variant.FeatureEnabled = true
return variant
}
return DISABLED_VARIANT
Expand Down
15 changes: 4 additions & 11 deletions api/variant.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ package api
import "github.com/Unleash/unleash-client-go/v4/context"

var DISABLED_VARIANT = &Variant{
Name: "disabled",
Enabled: false,
FeatureEnabled: false,
Name: "disabled",
Enabled: false,
}

type Payload struct {
Expand All @@ -27,11 +26,8 @@ type Variant struct {
Name string `json:"name"`
// Payload is the value of the variant payload
Payload Payload `json:"payload"`
// Enabled indicates whether the variant is enabled. This is only false when
// it's a default variant.
// Enabled indicates whether the feature which is extend by this variant was enabled or not.
Enabled bool `json:"enabled"`
// FeatureEnabled indicates whether the Feature for this variant is enabled.
FeatureEnabled bool `json:"featureEnabled"`
}

type VariantInternal struct {
Expand Down Expand Up @@ -85,10 +81,7 @@ func (o Override) matchValue(ctx *context.Context) bool {
return false
}

// Get default variant if feature is not found or if the feature is disabled.
//
// Rather than checking against this particular variant you should be checking
// the returned variant's Enabled and FeatureEnabled properties.
// Get default variant if no variant is found
func GetDefaultVariant() *Variant {
return DISABLED_VARIANT
}
12 changes: 2 additions & 10 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,6 @@ var defaultStrategies = []strategy.Strategy{
*s.NewFlexibleRolloutStrategy(),
}

// disabledVariantFeatureEnabled is similar to api.DISABLED_VARIANT but we want
// to discourage public usage so it's internal until there's a need to expose it.
var disabledVariantFeatureEnabled = &api.Variant{
Name: "disabled",
Enabled: false,
FeatureEnabled: true,
}

// Client is a structure representing an API client of an Unleash server.
type Client struct {
errorChannels
Expand Down Expand Up @@ -389,7 +381,7 @@ func (uc *Client) isParentDependencySatisfied(feature *api.Feature, context cont
func (uc *Client) GetVariant(feature string, options ...VariantOption) *api.Variant {
variant := uc.getVariantWithoutMetrics(feature, options...)
defer func() {
uc.metrics.countVariants(feature, variant.FeatureEnabled, variant.Name)
uc.metrics.countVariants(feature, variant.Enabled, variant.Name)
}()
return variant
}
Expand Down Expand Up @@ -444,7 +436,7 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt
}

if len(f.Variants) == 0 {
return disabledVariantFeatureEnabled
return defaultVariant
}

return api.VariantCollection{
Expand Down
89 changes: 0 additions & 89 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -740,8 +740,6 @@ func TestClient_VariantShouldRespectConstraint(t *testing.T) {

assert.True(variant.Enabled)

assert.True(variant.FeatureEnabled)

variantFromResolver := client.GetVariant(feature, WithVariantContext(context.Context{
Properties: map[string]string{"custom-id": "custom-ctx", "semver": "3.2.2", "age": "18", "domain": "unleashtest"},
}), WithVariantResolver(func(featureName string) *api.Feature {
Expand All @@ -755,8 +753,6 @@ func TestClient_VariantShouldRespectConstraint(t *testing.T) {

assert.True(variantFromResolver.Enabled)

assert.True(variantFromResolver.FeatureEnabled)

assert.True(gock.IsDone(), "there should be no more mocks")
}

Expand Down Expand Up @@ -859,8 +855,6 @@ func TestClient_VariantShouldFailWhenSegmentConstraintsDontMatch(t *testing.T) {

assert.False(variant.Enabled)

assert.False(variant.FeatureEnabled)

variantFromResolver := client.GetVariant(feature, WithVariantContext(context.Context{
Properties: map[string]string{"custom-id": "custom-ctx", "semver": "3.2.2", "age": "18", "domain": "unleashtest"},
}), WithVariantResolver(func(featureName string) *api.Feature {
Expand All @@ -874,8 +868,6 @@ func TestClient_VariantShouldFailWhenSegmentConstraintsDontMatch(t *testing.T) {

assert.False(variantFromResolver.Enabled)

assert.False(variantFromResolver.FeatureEnabled)

assert.True(gock.IsDone(), "there should be no more mocks")
}

Expand Down Expand Up @@ -961,8 +953,6 @@ func TestClient_ShouldFavorStrategyVariantOverFeatureVariant(t *testing.T) {

assert.True(strategyVariant.Enabled)

assert.True(strategyVariant.FeatureEnabled)

assert.Equal("strategyVariantName", strategyVariant.Name)

assert.True(gock.IsDone(), "there should be no more mocks")
Expand Down Expand Up @@ -1061,86 +1051,7 @@ func TestClient_ShouldReturnOldVariantForNonMatchingStrategyVariant(t *testing.T

assert.True(strategyVariant.Enabled)

assert.True(strategyVariant.FeatureEnabled)

assert.Equal("willBeSelected", strategyVariant.Name)

assert.True(gock.IsDone(), "there should be no more mocks")
}

func TestClient_VariantFromEnabledFeatureWithNoVariants(t *testing.T) {
assert := assert.New(t)
defer gock.OffAll()

gock.New(mockerServer).
Post("/client/register").
MatchHeader("UNLEASH-APPNAME", mockAppName).
MatchHeader("UNLEASH-INSTANCEID", mockInstanceId).
Reply(200)

feature := "feature-no-variants"
features := []api.Feature{
{
Name: feature,
Description: "feature-desc",
Enabled: true,
CreatedAt: time.Date(1974, time.May, 19, 1, 2, 3, 4, time.UTC),
Strategy: "default-strategy",
Strategies: []api.Strategy{
{
Id: 1,
Name: "default",
},
},
},
}

gock.New(mockerServer).
Get("/client/features").
Reply(200).
JSON(api.FeatureResponse{
Features: features,
Segments: []api.Segment{},
})

mockListener := &MockedListener{}
mockListener.On("OnReady").Return()
mockListener.On("OnRegistered", mock.AnythingOfType("ClientData"))
mockListener.On("OnCount", feature, true).Return()
mockListener.On("OnError").Return()

client, err := NewClient(
WithUrl(mockerServer),
WithAppName(mockAppName),
WithInstanceId(mockInstanceId),
WithListener(mockListener),
)

assert.NoError(err)
client.WaitForReady()

variant := client.GetVariant(feature, WithVariantContext(context.Context{}))

assert.False(variant.Enabled)

assert.True(variant.FeatureEnabled)

assert.Equal(disabledVariantFeatureEnabled, variant)

variantFromResolver := client.GetVariant(feature, WithVariantContext(context.Context{}), WithVariantResolver(func(featureName string) *api.Feature {
if featureName == features[0].Name {
return &features[0]
} else {
t.Fatalf("the feature name passed %s was not the expected one %s", featureName, features[0].Name)
return nil
}
}))

assert.False(variantFromResolver.Enabled)

assert.True(variantFromResolver.FeatureEnabled)

assert.Equal(disabledVariantFeatureEnabled, variantFromResolver)

assert.True(gock.IsDone(), "there should be no more mocks")
}
11 changes: 4 additions & 7 deletions unleash_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,14 @@ func Test_withVariants(t *testing.T) {
t.Fail()
}

variant := unleash.GetVariant("Demo")
if variant.Enabled == false {
t.Fatalf("Expected variant to be enabled")
}
if variant.FeatureEnabled == false {
feature := unleash.GetVariant("Demo")
if feature.Enabled == false {
t.Fatalf("Expected feature to be enabled")
}
if variant.Name != "small" && variant.Name != "medium" {
if feature.Name != "small" && feature.Name != "medium" {
t.Fatalf("Expected one of the variant names")
}
if variant.Payload.Value != "35" && variant.Payload.Value != "55" {
if feature.Payload.Value != "35" && feature.Payload.Value != "55" {
t.Fatalf("Expected one of the variant payloads")
}
err = unleash.Close()
Expand Down
Loading