Make endpoint and request options configurable. #28

robertatcd · 2021-04-01T00:50:07Z

These changes allow the client to be used with other Unleash servers.

My use case is GitLab: I need to be able to specify the feature flags endpoint for a particular project, and I need to pass two headers to identify the environment and the instance.

So in my own .env I have:

UNLEASH_URL="https://{our GitLab server}"
UNLEASH_FEATURES_ENDPOINT="/api/v4/feature_flags/unleash/{my project id}/features"

And in my config/unleash.php:

  'request_options' => [
    'headers' => [
        'UNLEASH-APPNAME' => 'local',
        'UNLEASH-INSTANCEID' => '{my instance id}',
    ]
  ],

mxkxf · 2021-04-08T08:15:03Z

Hi @robertatcd, thanks for this.

I'm just wondering how common this feature is?

If this is something very specific to you or your project, it may be better for you to maintain a fork of this.

robertatcd · 2021-04-08T09:18:18Z

Hi @mikefrancis

I found I needed to extend this in order to use the feature flag facility provided by GitLab, which I image will be fairly common.

It uses the same headers as specified in the Unleash docs:

https://docs.getunleash.io/docs/api/client/features

While developing this, I had assumed this was something that was mainly intended for use with servers that host flag sets for multiple projects, and so it would also be required if you were accessing the hosted service run by Unleash themselves:

https://www.unleash-hosted.com/open-source/

But looking at the docs again, I see they're not specific to that hosted service. They almost seem mandatory. So I'm wondering what contexts don't require them? I guess I'm missing something? (I'm new to Unleash and only starting to dabble in feature flags.)

robertatcd · 2021-04-08T09:35:05Z

Ah yes, here's the reason I thought the APPNAME and INSTANCEID headers were associated with the hosted Unleash service: they are configurable options in (at least some of) the SDKs mentioned on their site: https://www.unleash-hosted.com/demo/connect/#java

mxkxf · 2021-04-08T14:14:55Z

Ah ok, I see now - thanks for the docs!

I think rather than making the whole request options configurable, what if we set these 2 headers if the respected config options exist?

ppx17 · 2021-04-16T10:42:32Z

Explicit options are a bit more in line with how other clients (Java & Go) do it.In addition, the Java client also allows you to add custom headers, in case you host your instance behind a proxy that requires auth etc.

In my opinion, the way that is implemented in this PR is still very clear and easy to configure, so I would opt not to add the extra complexity of explicit options and then still needing this to provide potential additional headers.

PS. I'm also a Gitlab user that needs these precise options to use this library.

kalebdueck · 2021-04-16T17:54:30Z

This would be a great addition, coming from the gitlab side as well.

dshafik · 2021-04-26T01:08:16Z

I was also looking into this for our multi-application -> unleash setup: +1 here

mxkxf · 2021-04-26T10:03:15Z

My main reservation with the implementation here is that HTTP was originally completely separate from the Unleash service singleton and we're starting to see a lot of HTTP things bleeding into the Unleash service.

I think we may need to rethink where the HTTP options are coming from/can be configured.

Other than that, the reasoning makes complete sense - I did wonder why a lot of GitLab people were turning up 😅

robertatcd · 2021-05-09T09:12:55Z

(Apologies for not getting back to this until now).

@mikefrancis I'm still leaning towards the headers being configurable options. As @ppx17 commented, it makes them completely configurable. It should cover cases like proxies, other server implementations that need different headers, etc.

For the same reason, I made the API endpoint configurable rather than it being hardcoded to api/client/features, as that's needed in GitLab (for one example) to select between multiple projects.

I take your point about all the HTTP stuff being in the Unleash service though. Did you have anything in mind for refactoring it out?

mxkxf · 2021-05-10T08:18:51Z

@robertatcd I agree, let's make the headers configurable!

I think we could make use of the client initialisation here to set any headers that are set in config:

https://github.com/mikefrancis/laravel-unleash/blob/master/src/Client.php#L12-L16

Does that make sense?

evanshriner · 2021-05-25T14:41:30Z

definitely need this endpoint configurable in our setup. +1

mxkxf · 2021-06-01T07:56:10Z

config/unleash.php

@@ -5,6 +5,13 @@
  // This should be the base URL, do not include /api or anything else.
  'url' => env('UNLEASH_URL'),

+  // Endpoint for accessing feature flags, on the Unleash server.
+  'features_endpoint' => env('UNLEASH_FEATURES_ENDPOINT', '/api/client/features'),


We're using camelCase for the keys in this file. I don't think it's the actual Laravel convention but we should be consistent in this package

Addressed in ffee84d85bc676ac5309c7a6ca14b5c23f0642b7

…m/robertatcd/laravel-unleash into configurable_endpoint_and_options

+ move base_uri into requestDefaults config setting + use requestDefaults config setting when instantiating the client + retain the features endpoint as a separate config setting, in case the client is used to make different requests to different endpoints in the future.

robertatcd · 2021-06-11T09:22:54Z

Wow, where did that month go? Apologies for the long delay again.

@mikefrancis I hope this change is in line with what you were thinking, re: making use of the client initialisation.

I have bundled the url/base_uri setting into a general requestDefaults setting, which is used in the client initialisation.

At the suggestion of a colleague, I refrained from my initial approach of also tacking the features endpoint onto the end of base_uri, because that would make the features endpoint specifically the default for the client. His reasoning was that you may want to use that client to access other endpoints on the Unleash server in the future, so it's better to keep that config setting separate, and make use of it in the ->get() call in fetchFeatures().

mxkxf · 2021-06-14T12:23:59Z

src/Client.php

@@ -10,9 +10,7 @@ class Client extends GuzzleClient
    public function __construct(Config $config)
    {
        parent::__construct(
-            [


I think we may need to do a bit of merging of the config here as by removing the unleash.url we are not making this backwards compatible.

I am not sure this feature warrants an entire new major version, so I'm just wondering if there's anything clever we could do to fall back to unleash.url if unleash.requestDefaults doesn't exist? Does this make sense?

Oh yes, good point.

I have been hit twice this week by unheralded breaking changes, so I'm very sympathetic to that!

Updated to return unleash.url to its original spot.

mxkxf · 2021-06-14T12:26:13Z

@robertatcd Tell me about it! Thanks for your latest commits. The last comment is more of a discussion as it would be a breaking change but happy to merge after we've ironed that out.

robertatcd · 2021-06-19T01:34:32Z

src/Unleash.php

@@ -127,7 +127,7 @@ function () {

    protected function fetchFeatures(): array
    {
-        $response = $this->client->get($this->getFeaturesApiUrl(), $this->getRequestOptions());
+        $response = $this->client->get($this->config->get('unleash.featuresEndpoint'));


@mikefrancis I think this would be a breaking change too, actually, if someone used this update without knowing to add unleash.featuresEndpoint to their existing config file.

Might be best if I restore the getFeaturesApiUrl() function?

Ah, no. Not required, because of course you merge the solution's custom config with the package's config in your register method, which means that the default will be used even if the setting is left out of the custom config.

And I've tested and confirmed that theory.

mxkxf

Thank you @robertatcd ❤️

robertatcd · 2021-06-20T21:53:54Z

Thanks for the guidance @mikefrancis .

We're now using v0.7 to access GitLab feature flags in production, and it's working well!

Make endpoint and request options configurable.

d35dbb3

kalebdueck previously approved these changes Apr 16, 2021

View reviewed changes

Merge branch 'master' into configurable_endpoint_and_options

717c704

mxkxf reviewed Jun 1, 2021

View reviewed changes

robertatcd added 3 commits June 11, 2021 16:15

Merge branch 'master' into configurable_endpoint_and_options

70f8cb8

fix: camelCase for config settings.

ffee84d

Merge branch 'configurable_endpoint_and_options' of https://github.co…

4438d79

…m/robertatcd/laravel-unleash into configurable_endpoint_and_options

robertatcd dismissed kalebdueck’s stale review via 4438d79 June 11, 2021 04:20

robertatcd added 3 commits June 11, 2021 20:51

Reduce comment line length.

4ab6607

Update tests to reflect removal of requestOptions config setting.

15f97a9

mxkxf reviewed Jun 14, 2021

View reviewed changes

Return 'url' to its original place in config structure.

366f774

robertatcd commented Jun 19, 2021

View reviewed changes

mxkxf approved these changes Jun 19, 2021

View reviewed changes

mxkxf merged commit d370d33 into laravel-unleash:master Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make endpoint and request options configurable. #28

Make endpoint and request options configurable. #28

robertatcd commented Apr 1, 2021

mxkxf commented Apr 8, 2021

robertatcd commented Apr 8, 2021

robertatcd commented Apr 8, 2021

mxkxf commented Apr 8, 2021

ppx17 commented Apr 16, 2021 •

edited

Loading

kalebdueck commented Apr 16, 2021

dshafik commented Apr 26, 2021

mxkxf commented Apr 26, 2021 •

edited

Loading

robertatcd commented May 9, 2021

mxkxf commented May 10, 2021

evanshriner commented May 25, 2021

mxkxf Jun 1, 2021

robertatcd Jun 11, 2021

robertatcd commented Jun 11, 2021

mxkxf Jun 14, 2021

robertatcd Jun 19, 2021

mxkxf commented Jun 14, 2021

robertatcd Jun 19, 2021

robertatcd Jun 19, 2021

mxkxf left a comment

robertatcd commented Jun 20, 2021

Make endpoint and request options configurable. #28

Make endpoint and request options configurable. #28

Conversation

robertatcd commented Apr 1, 2021

mxkxf commented Apr 8, 2021

robertatcd commented Apr 8, 2021

robertatcd commented Apr 8, 2021

mxkxf commented Apr 8, 2021

ppx17 commented Apr 16, 2021 • edited Loading

kalebdueck commented Apr 16, 2021

dshafik commented Apr 26, 2021

mxkxf commented Apr 26, 2021 • edited Loading

robertatcd commented May 9, 2021

mxkxf commented May 10, 2021

evanshriner commented May 25, 2021

mxkxf Jun 1, 2021

Choose a reason for hiding this comment

robertatcd Jun 11, 2021

Choose a reason for hiding this comment

robertatcd commented Jun 11, 2021

mxkxf Jun 14, 2021

Choose a reason for hiding this comment

robertatcd Jun 19, 2021

Choose a reason for hiding this comment

mxkxf commented Jun 14, 2021

robertatcd Jun 19, 2021

Choose a reason for hiding this comment

robertatcd Jun 19, 2021

Choose a reason for hiding this comment

mxkxf left a comment

Choose a reason for hiding this comment

robertatcd commented Jun 20, 2021

ppx17 commented Apr 16, 2021 •

edited

Loading

mxkxf commented Apr 26, 2021 •

edited

Loading