Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add domain as an openfeature concept #229

Merged
merged 8 commits into from
Jan 24, 2024
Merged

feat: add domain as an openfeature concept #229

merged 8 commits into from
Jan 24, 2024

Conversation

beeme1mr
Copy link
Member

@beeme1mr beeme1mr commented Jan 10, 2024 •
edited
Loading

This PR

  • defines domain as a concept
  • updates references to client name to use domain
  • add domain as client metadata

Related Issues

Fixes #228

Follow-up Tasks

  • Update the readme template
  • Update the openfeature.dev SDK compatibility table

@beeme1mr
add namespace to spec
b5190a8 
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
beeme1mr
beeme1mr commented Jan 10, 2024
View reviewed changes
specification/glossary.md Outdated Show resolved Hide resolved
specification/sections/01-flag-evaluation.md Outdated Show resolved Hide resolved
@beeme1mr
fix ci issues
371b01b 
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
@beeme1mr
add non-normative text that explain the examples
f27627c 
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
@beeme1mr
fix linting issues
1887a92 
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
@dabeeeenster
Copy link
Contributor

Would this allow for multiple clients across multiple (or the same) provider?

@beeme1mr
Copy link
Member Author

Would this allow for multiple clients across multiple (or the same) provider?

Yes, exactly. The functionality already exists in many SDKs.

@beeme1mr beeme1mr mentioned this pull request Jan 10, 2024
Closed
@toddbaert
Copy link
Member

Would this allow for multiple clients across multiple (or the same) provider?

Yes, exactly. The functionality already exists in many SDKs.

Ya, it exists but is (IMHO) confusingly named, so I like this change.

@federicobond
Copy link
Member

I agree with the motivation, the current wording around named clients is a bit awkward. I don't think the word namespace makes things clear though. A namespace usually denotes a logical grouping of identifiers or symbols.

I propose we survey some examples of APIs solving similar problems to get a better idea of the alternatives.

As a first data point, the Python logging library uses "name" for the parameter to logging.getLogger which returns a logger with a specific configuration defined somewhere else.

@beeme1mr
Copy link
Member Author

@federicobond I thought namespace would be a good fit because it's a logical grouping of clients to a provider. However, I would happily change the proposal if we can find a more appropriate term.

@beeme1mr
Copy link
Member Author

Thanks for the feedback everyone. I've updated the PR to use the term domain. Please take a look when you have a moment.

@beeme1mr @toddbaert
Apply suggestions from code review
37feff9 
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
@beeme1mr
Copy link
Member Author

Thanks for the great reviews and feedback. I'll merge this on Wednesday the 24th unless there's an objection.

dominikhaska
dominikhaska approved these changes Jan 23, 2024
View reviewed changes
Copy link

@dominikhaska dominikhaska left a comment

Choose a reason for hiding this comment

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

Looks good - thank you :)

@beeme1mr beeme1mr merged commit afe1c7e into main Jan 24, 2024
6 checks passed
@beeme1mr beeme1mr deleted the provider-namespace branch January 24, 2024 13:49
@beeme1mr beeme1mr mentioned this pull request Jan 31, 2024
Merged
github-merge-queue bot pushed a commit to open-feature/js-sdk that referenced this pull request Feb 20, 2024
@beeme1mr @toddbaert
feat: add support for domains (#805)
98ba00a 
## This PR

- adds domain as a concept to the server and web SDK
- adds a deprecation warning anywhere client name was exposed to users.
- fixes an issue in the web SDK where context set on a domain before a
provider is registered was not used.

## Addresses

fixes #820
4aa9657

### Notes

This change is based on [this
spec](open-feature/spec#229) change. I tried to
make it a non-breaking change but I may have missed an untested
condition. Please carefully review to make sure I didn't miss anything.

### Follow-up Tasks

- Update the doc readme parser to support "domain".
- Update the NestJS and React SDKS. We should consider making those a
breaking change since they're sub 1.0.

---------

Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
@lukas-reining lukas-reining mentioned this pull request Feb 22, 2024
Merged
github-merge-queue bot pushed a commit to open-feature/js-sdk that referenced this pull request Feb 25, 2024
@lukas-reining @beeme1mr
feat(nestjs)!: add domains (#831)
840d7ac 
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

- adds support for domains
- removes named clients

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

relates to open-feature/spec#229

---------

Signed-off-by: Lukas Reining <lukas.reining@codecentric.de>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
This was referenced Mar 15, 2024
Closed
Closed
Closed
Closed
@beeme1mr beeme1mr mentioned this pull request Apr 25, 2024
Merged
@toddbaert toddbaert mentioned this pull request Nov 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@federicobond federicobond federicobond approved these changes

@dominikhaska dominikhaska dominikhaska approved these changes

@Kavindu-Dodan Kavindu-Dodan Kavindu-Dodan approved these changes

@sheepduke sheepduke sheepduke approved these changes

@thomaspoignant thomaspoignant thomaspoignant approved these changes

@austindrenski austindrenski austindrenski approved these changes

@toddbaert toddbaert toddbaert approved these changes

@lukas-reining lukas-reining lukas-reining approved these changes

@justinabrahms justinabrahms Awaiting requested review from justinabrahms

@weyert weyert Awaiting requested review from weyert

@moredip moredip Awaiting requested review from moredip

@dabeeeenster dabeeeenster Awaiting requested review from dabeeeenster

@thisthat thisthat Awaiting requested review from thisthat

@jonathannorris jonathannorris Awaiting requested review from jonathannorris

@benjiro benjiro Awaiting requested review from benjiro

@thiyagu06 thiyagu06 Awaiting requested review from thiyagu06

@kinyoklion kinyoklion Awaiting requested review from kinyoklion

@fabriziodemaria fabriziodemaria Awaiting requested review from fabriziodemaria

@luizgribeiro luizgribeiro Awaiting requested review from luizgribeiro

@liran2000 liran2000 Awaiting requested review from liran2000

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Redefine named clients as domain