fix: allow direct fetching of feature flags [IDE-1106] #322
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,27 +4,62 @@ import ( | |
| "github.com/snyk/go-application-framework/internal/api" | ||
| "github.com/snyk/go-application-framework/pkg/configuration" | ||
| "github.com/snyk/go-application-framework/pkg/workflow" | ||
| "sync" | ||
| ) | ||
|
|
||
| //go:generate $GOPATH/bin/mockgen -source=feature_flag.go -destination ../../mocks/feature_flag.go -package mocks -self_package github.com/snyk/go-application-framework/pkg/local_workflows/config_utils | ||
|
|
||
| func AddFeatureFlagToConfig(engine workflow.Engine, configKey string, featureFlagName string) { | ||
| config := engine.GetConfiguration() | ||
|
|
||
| callback := func(existingValue interface{}) (interface{}, error) { | ||
| if existingValue == nil { | ||
| return CurrentFeatureFlagChecker().GetFeatureFlag(engine, featureFlagName) | ||
| } else { | ||
| return existingValue, nil | ||
| } | ||
| } | ||
|
|
||
| config.AddDefaultValue(configKey, callback) | ||
| } | ||
|
|
||
| type FeatureFlagChecker interface { | ||
| GetFeatureFlag(engine workflow.Engine, featureFlagName string) (bool, error) | ||
| } | ||
|
|
||
| type featureFlagCheckerImpl struct { | ||
| } | ||
|
|
||
| var ( | ||
| currentFFC FeatureFlagChecker | ||
|
There was a problem hiding this comment. I don't like that we introduce a global variable here. Why is it needed? Can't we just have an instance in the configuration that is created when the configuration is created? |
||
| mutex = &sync.RWMutex{} | ||
| ) | ||
|
|
||
| func CurrentFeatureFlagChecker() FeatureFlagChecker { | ||
| mutex.RLock() | ||
| defer mutex.RUnlock() | ||
| if currentFFC == nil { | ||
| currentFFC = &featureFlagCheckerImpl{} | ||
| } | ||
| return currentFFC | ||
| } | ||
|
|
||
| func SetCurrentFeatureFlagChecker(ffc FeatureFlagChecker) { | ||
| mutex.Lock() | ||
| defer mutex.Unlock() | ||
| currentFFC = ffc | ||
| } | ||
|
Comment on lines
+38
to
+51
There was a problem hiding this comment. Why do we need a singleton pattern? |
||
|
|
||
| func (ffc *featureFlagCheckerImpl) GetFeatureFlag(engine workflow.Engine, featureFlagName string) (bool, error) { | ||
|
There was a problem hiding this comment. Suggestion: The singleton construct is not really used here, so I would recommend to just have a method There was a problem hiding this comment. I went for the overridable singleton solution like we do in because this seemed to me to be the way to allow for mocking. If there is a better way that means I can go back to just having the one function, then that would be good. For reference, here is how I am able to use the mocking with the overridable singleton solution: There was a problem hiding this comment. maybe instead of a singleton the fetureFlagChecker can be set on the engine lvl There was a problem hiding this comment. sorry, it seems I missed any notification on this thread. is this still something we are looking to merge? There was a problem hiding this comment. @PeterSchafer Yes, still looking to merge, but there isn't a rush for it anymore, so can be cooldown. Would you prefer I set it on the function on the engine level as Shawky suggested? There was a problem hiding this comment. Haha, I see, everyone already commented on that. I think it's a bit overengineered. Having an interface for mocking is fine, but IMHO a singleton is not needed. I'd implement the interface in Configuration as it's in the configuration domain, not the engine. |
||
| config := engine.GetConfiguration() | ||
| httpClient := engine.GetNetworkAccess().GetHttpClient() | ||
|
There was a problem hiding this comment. I wonder whether it would be better to have config & httpclient injected, to decouple and avoid side-effects (same like it's done with apiClient). |
||
| logger := engine.GetLogger() | ||
| url := config.GetString(configuration.API_URL) | ||
| org := config.GetString(configuration.ORGANIZATION) | ||
| apiClient := api.NewApi(url, httpClient) | ||
| result, err := apiClient.GetFeatureFlag(featureFlagName, org) | ||
| if err != nil { | ||
| logger.Printf("Failed to determine feature flag \"%s\" for org \"%s\": %s", featureFlagName, org, err) | ||
| } | ||
| return result, nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice :)