Skip to content

docs(ADR): support flags with duplicate keys.#1660

Merged
toddbaert merged 6 commits intoopen-feature:mainfrom
tangenti:tangent/flag-list
Jul 8, 2025
Merged

docs(ADR): support flags with duplicate keys.#1660
toddbaert merged 6 commits intoopen-feature:mainfrom
tangenti:tangent/flag-list

Conversation

@tangenti
Copy link
Contributor

This is proposal for support flags with duplicate keys, as a follow up of the discussion on #1634 and #1644.

The selector semantics change proposed in #1644 will be addressed in a separate ADR.

Signed-off-by: Hugo Huang <lorqor@gmail.com>
@tangenti tangenti requested a review from a team June 30, 2025 19:11
@tangenti tangenti requested a review from a team as a code owner June 30, 2025 19:12
@netlify
Copy link

netlify bot commented Jun 30, 2025

Deploy Preview for polite-licorice-3db33c canceled.

Name Link
🔨 Latest commit fbbeab8
🔍 Latest deploy log https://app.netlify.com/projects/polite-licorice-3db33c/deploys/686baff68c23c00008348cd8

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 30, 2025
Signed-off-by: Hugo Huang <lorqor@gmail.com>
@tangenti tangenti changed the title ADR for allowing flags with duplicate keys. Adocs(ADR): support flags with duplicate keys. Jun 30, 2025
@tangenti tangenti changed the title Adocs(ADR): support flags with duplicate keys. docs(ADR): support flags with duplicate keys. Jun 30, 2025
@toddbaert toddbaert assigned juanparadox and unassigned juanparadox Jul 1, 2025
@toddbaert toddbaert requested a review from juanparadox July 1, 2025 15:19
@toddbaert
Copy link
Member

toddbaert commented Jul 1, 2025

I see this proposal as totally workable, but in conflict with #1634 .

In my mind, the main choice we have here is between the more opinionated, less flexible hierarchical structure of #1634, which comes along with built-in assurances (such as the logical impossibility of having duplicate keys within a set) but makes "flagSetId" a special "key-of-keys" property and invalidates the previous usage of flagSetId, vs this proposal, which is less breaking and more flexible. With this proposal, the existing semantics of flagSetId don't change at all, but we now have to handle a new type of conflict (conflicts within the logical keys created by flagSetId + flagKey).

I think I prefer this proposal slightly because of the non-breaking aspects.

Signed-off-by: Hugo Huang <lorqor@gmail.com>
@toddbaert toddbaert self-requested a review July 3, 2025 19:17
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After consideration and discussions internally, I think I'm in favor of accepting this over #1634 due to it's flexibility and "less-breaking" nature.

tangenti added 2 commits July 4, 2025 11:20
Signed-off-by: Hugo Huang <lorqor@gmail.com>
@tangenti
Copy link
Contributor Author

tangenti commented Jul 4, 2025

After consideration and discussions internally, I think I'm in favor of accepting this over #1634 due to it's flexibility and "less-breaking" nature.

Thanks a lot Todd. As Guido pointed out in the comments, I think we could consider different options for the storage layer implementation.

@guidobrei Could you approve the PR if there's no more concerns from you and you believe we're aligned here?

@tangenti
Copy link
Contributor Author

tangenti commented Jul 8, 2025

@toddbaert It seems I don't have the permission to merge the PR. Could you merge the PR if everything looks good to you?

@toddbaert toddbaert merged commit 797b482 into open-feature:main Jul 8, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants