feat: add configuration property to define which key is used if a string without equals is used as a selector

[aepfli]

This pull request introduces a significant enhancement to how flag selectors are parsed, particularly for simplified selector strings that do not explicitly define a key-value pair. By adding a configurable fallback key, the system gains increased flexibility, allowing users to customize the interpretation of such selectors beyond the default 'source' key. This change improves the adaptability of the flag evaluation logic without breaking backward compatibility.

Simply put: if we have a selector without a = we are currently assume that this is a "source"-selector and transform it to source=<selector>. With this changes, we can configure the key of this selector to any arbitrary key. So when selector-fallback-key is foo this will turn it into foo=<selector>.

Highlights

• Configurable Selector Fallback: Introduced a new configuration property (selector-fallback-key) that allows defining which key is used when a selector string without an equals sign is provided (e.g., "myValue" instead of "key=myValue").
• Enhanced Selector Parsing: The internal Selector and expressionToMap functions now support this configurable fallback, defaulting to the existing sourceIndex if no fallback key is specified.
• Store Initialization Refactor: The store.NewStore function now accepts a StoreConfig struct, encapsulating source and the new selector fallback key, leading to cleaner initialization.
• Command-Line Option: A new --selector-fallback-key flag has been added to the flagd command to allow users to specify this fallback behavior via command-line arguments.

[netlify]

✅ Deploy Preview for polite-licorice-3db33c canceled.

Name	Link
🔨 Latest commit	9ce0181
🔍 Latest deploy log	https://app.netlify.com/projects/polite-licorice-3db33c/deploys/68d65896f323bc0008ba1c42

[gemini-code-assist]

Summary of Changes

Hello @aepfli, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a significant enhancement to how flag selectors are parsed, particularly for simplified selector strings that do not explicitly define a key-value pair. By adding a configurable fallback key, the system gains increased flexibility, allowing users to customize the interpretation of such selectors beyond the default 'source' key. This change improves the adaptability of the flag evaluation logic without breaking backward compatibility.

Highlights

• Configurable Selector Fallback: Introduced a new configuration property (selector-fallback-key) that allows defining which key is used when a selector string without an equals sign is provided (e.g., "myValue" instead of "key=myValue").
• Enhanced Selector Parsing: The internal Selector and expressionToMap functions now support this configurable fallback, defaulting to the existing sourceIndex if no fallback key is specified.
• Store Initialization Refactor: The store.NewStore function now accepts a StoreConfig struct, encapsulating source and the new selector fallback key, leading to cleaner initialization.
• Command-Line Option: A new --selector-fallback-key flag has been added to the flagd command to allow users to specify this fallback behavior via command-line arguments.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[aepfli]

/gemini review

last review from gemini, rest is for the community

[gemini-code-assist]

Code Review

This pull request introduces a configurable fallback key for flag selectors, which is a great enhancement for flexibility. The implementation is solid, with good refactoring of the store initialization and extensive test coverage for the new selector parsing logic. I have a few suggestions to improve robustness and clarity.

[toddbaert]

@aepfli staticcheck says: S1003: should use !strings.Contains(sExp, "=") instead (staticcheck), which makes sense to me.

[toddbaert]

I don't think we need this member (per Selector), and we don't need a separate "With" for it either, since this is an immutable global setting (all Selectors will share it).

Instead, I would recommend taking advantage of module initialization and sync.Once to do this, by adding something like this at the top of the file (just below the imports):

var fallbackKey string // our fallback key global
var fallbackKeyOnce sync.Once // sync.Once instance

// public function
func SetFallbackKey(key string) {
	fallbackKeyOnce.Do(func() {
		fallbackKey = key
	})
}

Then we just call SetFallbackKey at some point during init, which sets the fallback key ONLY ONCE for the duration of the program; then we can get rid of this per instance and just always refer to this module global.

[toddbaert]

BTW for testing, you can manually reset the module private sync.Once.

[toddbaert]

I don't think we need specific benchmark tests for this.

[toddbaert]

We can remove all these tests (perhaps moving 1 or 2 to the other functions) if we do this.

[toddbaert]

This was mostly to facilitate an internal migration issue, but we've come up with an alternative that will avoid adding this to flagd.

[aepfli]

I am not sure I agree here, yes the motivation stems from an internal problem, and we might be able to fix this differently internally, but the problem still exists for other users of flagd, which also do need some kind of migration path.

How should they handle this?

[toddbaert]

I am not sure I agree here, yes the motivation stems from an internal problem, and we might be able to fix this differently internally, but the problem still exists for other users of flagd, which also do need some kind of migration path.

How should they handle this?

This is not a problem for external users. A selector without an expression will continue to represent a source, there's no plan to change this, and no need to change it, since up to now is been the only possibility.

[aepfli]

i am not sure i am agreeing with this comment, because flagset will be in the future the default approach, and as a library/service we should provide a migration path also for other projects. But it is not he right approach to this via this pr implicitly, we should do this explicitly with also defining the migration and deprecation path. In the end this is also something like the experimental flags for the jvm, till a feature reaches GA and afterwards they get removed again. We should make this migration easy for users of flagd to also rely on the flagset as a default, without forcing their sdk users, to update their dependencies.

[toddbaert]

i am not sure i am agreeing with this comment, because flagset will be in the future the default approach, and as a library/service we should provide a migration path also for other projects. But it is not he right approach to this via this pr implicitly, we should do this explicitly with also defining the migration and deprecation path. In the end this is also something like the experimental flags for the jvm, till a feature reaches GA and afterwards they get removed again. We should make this migration easy for users of flagd to also rely on the flagset as a default, without forcing their sdk users, to update their dependencies.

Nobody who is using flagd right now is using the selector for a flagSetId because it's not possible, so there's no need for a "migration path". Anyone who wants to select on a flagSetId can use an expression to do that in the future, so I'm not seeing the point.