fix: allow direct fetching of feature flags [IDE-1106]

[rrama]

Previously all feature flags had to be registered with the GAF configuration before they could be used.

[rrama]

I need this for https://github.com/snyk/snyk-ls/pull/820/files#diff-d74ed587c3dae58e0c1ddc2a0eec71bdb58552ef0b6852d664fbb849e868ec19R59 so I can directly fetch feature flags using GAF's API.
I will be calling:

config_utils.CurrentFeatureFlagChecker().GetFeatureFlag(config.CurrentConfig().Engine(), ffStr)

I needed to mock the fetching of feature flags for the test (https://github.com/snyk/snyk-ls/pull/820/files#diff-29de3939a8441248dd44aa2f2eb09f23a57909cf126d85840d0a1519580e25f3R37), which is why I did the horrible stuff with and interface and struct. If there is a better way to do it, then please let me know, it's my first time using mocks in Go.

[PeterSchafer]

I need this for https://github.com/snyk/snyk-ls/pull/820/files#diff-d74ed587c3dae58e0c1ddc2a0eec71bdb58552ef0b6852d664fbb849e868ec19R59 so I can directly fetch feature flags using GAF's API. I will be calling:

config_utils.CurrentFeatureFlagChecker().GetFeatureFlag(config.CurrentConfig().Engine(), ffStr)

I needed to mock the fetching of feature flags for the test (https://github.com/snyk/snyk-ls/pull/820/files#diff-29de3939a8441248dd44aa2f2eb09f23a57909cf126d85840d0a1519580e25f3R37), which is why I did the horrible stuff with and interface and struct. If there is a better way to do it, then please let me know, it's my first time using mocks in Go.

Question: I think I'm not yet understanding the use case or the problem you are solving well enough. Could you please explain this a bit more?

[rrama]

Question: I think I'm not yet understanding the use case or the problem you are solving well enough. Could you please explain this a bit more?

Certainly, for some context, in the language server we have the LSP command snyk.getFeatureFlagStatus, this allows a client to request the status of any feature flag by it's name. Currently the language server has its own code to fetch feature flags (and SastSettings) from Snyk's backend API. We are wanting to deprecate the language server having its own API client and instead make it use GAF's API client.

Now I am trying to rewrite the snyk.getFeatureFlagStatus command using GAF's API client. I didn't realise that Configuration.GetBool(key string) for feature flags used a key that is internal to GAF (e.g. FF_CODE_CONSISTENT_IGNORES = "internal_snyk_code_ignores_enabled"), which is mapped to the real feature flag name using:

config_utils.AddFeatureFlagToConfig(engine, configuration.FF_CODE_CONSISTENT_IGNORES, "snykCodeConsistentIgnores")

For the snyk.getFeatureFlagStatus command to work, it needs to bypass this internal key name mapping to feature flag name code, otherwise we won't be able to fetch feature flags unless they are already explicitly known to GAF (plus we'd have to create a reverse map of feature flag names to the internal key name to then be able to then call GetBool).

Now whether this is the best solution for bypassing the internal config key names or not, I don't know. The code was a lot simpler before I added the ability to mock the response so that I could get the unit test for the snyk.getFeatureFlagStatus command to work. I have ended up spewing a lot of bloat code in that is horrible :'(

Another option would be to add a function Configuration.GetFeatureFlag(ffName string) (bool, error) but that could be cause confusion down the line when someone tries to do GetFeatureFlag(configuration.FF_CODE_CONSISTENT_IGNORES).

[PeterSchafer]

Thank you @rrama ! That is very helpful.

[PeterSchafer]

Suggestion: The singleton construct is not really used here, so I would recommend to just have a method GetFeatureFlag() without the whole singleton.

[rrama]

I went for the overridable singleton solution like we do in

go-application-framework/pkg/workflow/engineimpl.go

Line 53 in f23723b

func (e *EngineImpl) SetConfiguration(config configuration.Configuration) {

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:
snyk/snyk-ls@e94e241#diff-29de3939a8441248dd44aa2f2eb09f23a57909cf126d85840d0a1519580e25f3R46

[ShawkyZ]

maybe instead of a singleton the fetureFlagChecker can be set on the engine lvl engine.SetFeatureFlagChecker. or smth similar ? That way tests and other clients can set the mock.
cc @PeterSchafer

[PeterSchafer]

sorry, it seems I missed any notification on this thread. is this still something we are looking to merge?

[rrama]

@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?

[bastiandoetsch]

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.

[PeterSchafer]

Just to summarize my understanding.

Goal: We want to enable accessing feature flags whose names are not yet known at compile time.

1. After re-reading the following part of the comment above

For the snyk.getFeatureFlagStatus command to work, it needs to bypass this internal key name mapping to feature flag name code, otherwise we won't be able to fetch feature flags unless they are already explicitly known to GAF (plus we'd have to create a reverse map of feature flag names to the internal key name to then be able to then call GetBool).

I think there is a misunderstanding. the configuration key and the feature flag name can be the same, there is no such thing as an internal key required. Only if you want to support setting a feature flag remotely and via other means like environment variables, a mapping is helpful. But this is not what you need.

2. For your use case, you could also just not use the configuration at all, since you are just interested in the value based on a name. You could replace the snyk-ls api client by the gaf api client. The only change it would need would be to expose this api client.

I hope this makes sense @rrama. If I'm missing something here, please let's jump on a call to get to a resolution.
And again sorry for the delay on this topic.

[bastiandoetsch]

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?

[bastiandoetsch]

Why do we need a singleton pattern?

[bastiandoetsch]

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).

[bastiandoetsch]

nice :)

[rrama]

I am going to move this to a draft for now until the next cooldown, then I will look closer at the solutions suggested and then we can pick which works best.

[PeterSchafer]

Since we might need the same functionality in another use case, I went ahead and opened #349 which should do what you need. Let me know if this makes sense.