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

feat!: Updated service to use connect #163

Merged
merged 26 commits into from
Sep 26, 2022
Merged

feat!: Updated service to use connect #163

merged 26 commits into from
Sep 26, 2022

Conversation

james-milligan
Copy link
Contributor

Connect is a family of libraries for building browser and gRPC-compatible HTTP APIs. It is backwards compatible with gRPC and allows for all currently listed service providers and their 'variations' (gRPC, http(s), unix / tcp) to be managed by a single service provider with a single set of handlers.

Using connect introduces the connect protocol, directly compatible with the connect-go andf connect-web client libraries. This provides the benefit avoiding the use of heavy libraries such as grpc-web in the front end applications. It is also fully compatible with gRPC meaning that any existing clients that match the interface will still work, thus, providers can still be writen for other languages using pure grpc clients, without requiring a language specifc conenct library.

Breaking changes:

The http routes are changed, there is currently no support for providing 'custom' routes. A request which previously will have looked like this:

$ curl -X POST "localhost:8013/flags/headerColor/resolve/string" -d '{"email": "foo@bar.com"}'
{"value":"#0000FF","reason":"TARGETING_MATCH","variant":"blue"}

will not look like this:

$ curl -X POST "localhost:8013/schema.v1.Service/ResolveString" -d ''{"flagKey":"headerColor","context":{"email": "foo@bar.com"}}'' -H "Content-Type: application/json"
{"value":"#0000FF","reason":"TARGETING_MATCH","variant":"blue"}

This may not be an issue going forwards as these URLs are internal, and will only requre an update to the provider.
route prefixes can be added to allow for /api/ to be appended to the front of each route.

There is no longer a --service-provider option, this is because all variations of this flagkey are now available through the single connect service. (there may be argument for keeping this flag and setting the default to 'connect')

Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
james-milligan and others added 3 commits September 20, 2022 15:08
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
@toddbaert
Copy link
Member

toddbaert commented Sep 21, 2022

I'll look at this first thing tomorrow. I'd like to get this in.

Although the benchmark does seem to be hanging for some reason, and there's errors in it's logs.

@AlexsJones
Copy link
Member

Nice reduction in code, happy times

Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
@james-milligan
Copy link
Contributor Author

Although the benchmark does seem to be hanging for some reason, and there's errors in it's logs.

I have resolved the error log on the benchmarks, the hanging seems to be on writing to the gh-pages branch so i will look into this

Copy link
Contributor

@skyerus skyerus left a comment

Choose a reason for hiding this comment

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

Nice work :)

pkg/service/connect_service.go Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
@toddbaert
Copy link
Member

toddbaert commented Sep 21, 2022

I tested this locally with the current js flagd provider, behavior looks the same as before (which is good!). Happy paths all work as expected.

The only issue I'm having (which existed before this change) is that it's a bit messy to map the RPCErrors to our error codes. See the screenshot below for an example RPC error for a not-found flag:

image

As you can see the code is a standard RPC error code NOT_FOUND, which makes sense. The message is a string with that code appended to our SDK error code FLAG_NOT_FOUND. I think it would be great if we could have the message contain only our error code, so flagd providers could easily set the appropriate SDK error code without manipulating this string.

What do you think? Is that possible?

UPDATE: we think this might be a parsing issue in the clients. Either way, this isn't a blocker for this PR since it's not new behavior.

@toddbaert
Copy link
Member

@james-milligan You've missed updating a couple old REST examples in the readme.

Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
Signed-off-by: James-Milligan <james@omnant.co.uk>
@toddbaert toddbaert changed the title refactor!: Updated service to use connect feat!: Updated service to use connect Sep 22, 2022
@AlexsJones
Copy link
Member

What does the feat! mean @toddbaert is this not ready for merge?

@toddbaert
Copy link
Member

toddbaert commented Sep 23, 2022

@AlexsJones

What does the feat! mean

feat! is just a semantic convention for git commits.

! means breaking, and can be applied to any commit prefix, but only feat and fix are user-facing and will appear in the changelog automatically. I don't consider this a "refactor" because it makes user-visible changes (REST endpoints, etc) so it's not a refactor.

@toddbaert is this not ready for merge?

I'll leave that to @james-milligan . It looks good to me!

Signed-off-by: James-Milligan <james@omnant.co.uk>
@AlexsJones AlexsJones merged commit 828d5c4 into open-feature:main Sep 26, 2022
AlexsJones pushed a commit that referenced this pull request Sep 26, 2022
🤖 I have created a release *beep* *boop*
---


##
[0.2.0](v0.1.1...v0.2.0)
(2022-09-26)


### ⚠ BREAKING CHANGES

* Updated service to use connect (#163)

### Features

* Updated service to use connect
([#163](#163))
([828d5c4](828d5c4))


### Bug Fixes

* checkout release tag before running container and binary releases
([#171](#171))
([50fe46f](50fe46f))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@james-milligan james-milligan deleted the connect-refactor branch March 23, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants