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.

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

match is not returning #60

Closed
andreyvital opened this issue May 10, 2019 · 14 comments · Fixed by #66
Closed

match is not returning #60

andreyvital opened this issue May 10, 2019 · 14 comments · Fixed by #66
Labels

Comments

@andreyvital
Copy link

Hey @markphelps—great work.

I'm thinking about using flipt for a product I'm developing, however, I am not sure if I'm doing something wrong.

I've created one flag called "test" with a segment that matches on all contexts and a targeting rule on set to 50% and off also set to 50%. I can't seem to get a match: true or match: false back from the server.

@andreyvital
Copy link
Author

echo '{"flag_key":"test","entity_id":"1","request_id":"1","context":{}}' |http http://localhost:8080/api/v1/evaluate
HTTP/1.1 200 OK
Content-Encoding: gzip
Content-Length: 133
Content-Type: application/json
Date: Fri, 10 May 2019 23:20:56 GMT
Grpc-Metadata-Content-Type: application/grpc

{
    "entityId": "1",
    "flagKey": "test",
    "requestDurationMillis": 0.214472,
    "requestId": "1",
    "timestamp": "2019-05-10T23:20:56.536974628Z"
}

@markphelps markphelps added the bug label May 11, 2019
@markphelps
Copy link
Collaborator

markphelps commented May 11, 2019

Thanks! Glad you find it useful.

I think this might be a use case that I overlooked. Do you have any constraints defined for your segment or no?

I'm assuming no since a segment with no constraints should potentially match everything.

@andreyvital
Copy link
Author

@markphelps hmm...well, I want to segment on all users, but then on variant would match for 50% and off would match for other half. Am I misunderstanding something? 🤔

@markphelps
Copy link
Collaborator

Nope I think it’s just an oversight on my end. Will try to fix this weekend. Would you mind posting a screenshot of your segment?

@andreyvital
Copy link
Author

image
image

@andreyvital
Copy link
Author

it'd be awesome if you manage to fix it...i'm really excited to start using it in my product! I think it simplifies feature flagging and abstracts the way out of complex implementations

🍻 💯 😍

@markphelps
Copy link
Collaborator

Thanks! Will work on it this weekend. Would love to talk to you as well about how you plan to use Flipt in your product.

@markphelps
Copy link
Collaborator

So I've reproduced the issue and have a workaround for you while I think more about if this is really a bug or a design choice.

Background

Here's some info on why this isn't working as expected:

Rules are attached to flags and segments, but in order for a rule to match a segment, currently a segment has to have constraints. This is so it can have something to match against your segments, and figure out which rule it should follow.

The more I think about it, I think it does make sense to allow a segment to match on no constraints, like you expect. It will just require a bit of work for me to redo some of the evaluation logic, so it make take a few days, so I wanted to provide you a work around in the meantime.

Workaround

Screen Shot 2019-05-11 at 9 56 49 AM

Add a constraint to your segment, with a boolean type and an operator of NOT PRESENT. The property doesnt matter.

This should allow your rule to evaluate as you would expect.

I'll follow up shortly with more info on the real fix

@andreyvital
Copy link
Author

@markphelps yeah I've done that, but feels weird to me to always pass something in the context just to force it to match a segment

@markphelps
Copy link
Collaborator

markphelps commented May 11, 2019 via email

@markphelps
Copy link
Collaborator

Actually, with the workaround I described, you shouldn't have to pass anything in the context since the constraint is "field IS NOT PRESENT".

Screen Shot 2019-05-11 at 2 00 48 PM

@andreyvital
Copy link
Author

Yeah thats right 🤔

So...you think you'll be able to put together a fix that doesn't require me to define a rule to match all or should I go ahead with this workaround?

Define a random key that'll never be present.

@markphelps
Copy link
Collaborator

Yup will still work on fixing this so that you don't have to define a 'fake' constraint

@markphelps
Copy link
Collaborator

@andreyvital this should be fixed in the latest release, please let me know if you have any more issues.

Thanks again 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants