-
Notifications
You must be signed in to change notification settings - Fork 15
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.
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
feat!: Support transient identities and traits #133
Conversation
client.go
Outdated
type GetIdentityFlagsOpts struct { | ||
Transient bool `json:"transient,omitempty"` | ||
} |
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.
@gagantrivedi Keen to know if this approach is redundant. Maybe we could just go with a transient bool
argument to respective interfaces.
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.
Yeah, I'd prefer the latter approach
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.
Having implemented the bool
argument, I don't quite like not being able to provide nil
as the value. Since Golang doesn't support optional args, I feel we're forcing the majority of users to make the decision they don't need to make.
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.
I think a bool pointer should be able to achieve that?
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.
It should, in theory, but then there's no way to provide a pointer to a constant (that I know of, at least).
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.
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.
Hmm... yep, ok, that's a good point. Perhaps, for now, we should add that method and mark getEnvironmentFlags
and getIdentityFlags
as deprecated? To use transient identities, you will need to move to use the getFlags
method.
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.
I also like implementing the OF interface with an evaluation context that can contain the identity. Actually, an immediate benefit of doing that is that we could optionally send the environment or another environment key in this call providing an easy way to "switch context" I guess, but that's just an example and another discussion.
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.
Annoyingly I don't think it'll be as straight forward on the client-side with OF. In JS there's a single OF context. I wonder if this could be achieved using their concept of domains,
const domainScopedClient = OpenFeature.getClient("my-domain");
domainScopedClient.setContext(context1)
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.
@kyle-ssg Domain-specific contexts look like the exact solution for this.
ec66647
to
7435704
Compare
d91e2a4
to
511d1e5
Compare
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.
One minor comment, but looks good to me.
@@ -0,0 +1,8 @@ | |||
.EXPORT_ALL_VARIABLES: | |||
|
|||
EVALUATION_CONTEXT_SCHEMA_URL ?= https://raw.githubusercontent.com/Flagsmith/flagsmith/main/sdk/evaluation-context.json |
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.
Slightly existential question, but should this point at main
or a tag?
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.
The schema is designed to be forward-compatible so main
is pretty safe. That being said, we don't have any automation around it in CI/CD for Golang SDK yet.
Closes #132.