From c42351cf7686ac9d56c70dcf438637d115a9d603 Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Wed, 13 Dec 2023 09:27:15 +0100 Subject: [PATCH] Revert "feat: add FeatureEnabled to Variant (#164)" (#165) This reverts commit 170cce6bfbb35722e7fb53ab87e6628976da0e7b. --- api/feature.go | 1 - api/variant.go | 15 +++------ client.go | 12 ++----- client_test.go | 89 ------------------------------------------------- unleash_test.go | 11 +++--- 5 files changed, 10 insertions(+), 118 deletions(-) diff --git a/api/feature.go b/api/feature.go index 1ab1e1e..8bd8c04 100644 --- a/api/feature.go +++ b/api/feature.go @@ -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 diff --git a/api/variant.go b/api/variant.go index 8211dd2..bf8b229 100644 --- a/api/variant.go +++ b/api/variant.go @@ -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 { @@ -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 { @@ -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 } diff --git a/client.go b/client.go index b757de6..a2a6c57 100644 --- a/client.go +++ b/client.go @@ -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 @@ -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 } @@ -444,7 +436,7 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt } if len(f.Variants) == 0 { - return disabledVariantFeatureEnabled + return defaultVariant } return api.VariantCollection{ diff --git a/client_test.go b/client_test.go index 8950ce6..ba5331c 100644 --- a/client_test.go +++ b/client_test.go @@ -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 { @@ -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") } @@ -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 { @@ -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") } @@ -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") @@ -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") -} diff --git a/unleash_test.go b/unleash_test.go index 1e96730..1bf07a0 100644 --- a/unleash_test.go +++ b/unleash_test.go @@ -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()