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

Move retriever / notifier / exporter on their own packages. #264

Merged
merged 13 commits into from
Jul 7, 2022

Conversation

thomaspoignant
Copy link
Owner

@thomaspoignant thomaspoignant commented Jul 4, 2022

Description

⚠️ Breaking changes ⚠️

This version change the way to do your SDK initialization in particular when it comes to notifier, provider and exporter.

Why are we doing this breaking changes?

This change is needed because we are adding more and more retriever / exporter and notifier, and with that we are also adding more dependencies to the project.
And since all the dependencies are in the same package we were increasing the size of your build even if you were not using the new providers.

The best example is when we introduce the kubernetes provider, it has added ~20Mo in your application because the client-go from kubernetes is huge.

What did we change?

We have moved all retriever / exporter and notifier in a dedicated package for each of them.
With this new organisation when building your app we will use only the dependencies related to your configuration.

We also change the way to deal with the notifiers to align it with the way it work for other extensions.

How to migrate?

Edit your init function of go-feature-flag and replace your retriever, exporter, notifier following this.

Retrievers

ffclient.FileRetriever          ->      fileretriever.Retriever
ffclient.GithubRetriever        ->      gcstorageretriever.Retriever
ffclient.GithubRetriever        ->      githubretriever.Retriever
ffclient.HTTPRetriever          ->      httpretriever.Retriever
ffclient.KubernetesRetriever    ->      k8sretriever.Retriever
fflcient.S3Retriever            ->      s3retriever.Retriever

Exporters

ffexporter.File                 ->      fileexporter.Exporter
ffexporter.GoogleCloudStorage   ->      gcstorageexporter.Exporter
ffexporter.Log                  ->      logsexporter.Exporter
ffexporter.S3                   ->      s3exporter.Exporter
ffexporter.Webhook              ->      webhookexporter.Exporter

Notifiers

Since in this PR we change the way to work with notifier, you have more impact when configuring them.

Before this PR to configure a notifier you had something like that.

_, err := ffclient.New(ffclient.Config{
    // ...
    Notifiers: []ffclient.NotifierConfig{
        &ffclient.WebhookConfig{
            // ...
        },
    },
})

With this PR we had remove the ffclient.NotifierConfig struct to use directly the notifier them self.
It means that now ffclient.Config.Notifiers has the type []notifier.Notifier.
So it will look like:

_, err := ffclient.New(ffclient.Config{
    // ...
    Notifiers: []notifier.Notifier{
        &webhooknotifier.Notifier{
            // ...
        },
    },
})

To follow the same pattern has the retrievers and exporters we also have changed the struct you were using.

notifier.LogNotifier        ->      logsnotifier.Notifier
ffclient.SlackNotifier      ->      slacknotifier.Notifier
ffclient.WebhookConfig      ->      webhooknotifier.Notifier

Changes include

  • Breaking changes (change that is not backward-compatible and/or changes current functionality)

Closes issue(s)

Resolve #256

Checklist

  • I have tested this code
  • I have added unit test to cover this code
  • I have updated the documentation (README.md and /docs)
  • I have followed the contributing guide

@coveralls
Copy link

coveralls commented Jul 4, 2022

Coverage Status

Coverage increased (+0.4%) to 94.381% when pulling 691f2e1 on feat/move-retriever into e876781 on main.

@thomaspoignant thomaspoignant marked this pull request as ready for review July 6, 2022 09:55
Signed-off-by: Thomas Poignant <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
@thomaspoignant thomaspoignant self-assigned this Jul 6, 2022
@thomaspoignant thomaspoignant added this to the 1.0.0 milestone Jul 6, 2022
Signed-off-by: Thomas Poignant <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
@urso
Copy link

urso commented Jul 6, 2022

Thank you, the PR description is very detailed.

I only skimmed over the changes, but LGTM. Only nit is that the placing of the testdata feels a little non-idiomatic. As the exporters/retrievers/notifiers are more isolated now I would normally expect the testdata right next to each exporter. Like exporter/file/testdata and so on.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.4% 1.4% Duplication

@thomaspoignant thomaspoignant merged commit e3fb234 into main Jul 7, 2022
@thomaspoignant thomaspoignant deleted the feat/move-retriever branch July 7, 2022 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(change) Move retrievers into sub-pacakges (or external libs?)
3 participants