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

refactor(deriv_feature_flag): DERG-1561 Integration_of_Growthbook #444

Closed

Conversation

ilya-deriv
Copy link
Contributor

@ilya-deriv ilya-deriv commented Feb 6, 2024

Clickup link:
https://deriv-group.slack.com/archives/C04U81FL9D4/p1707119424046629

This PR contains the following changes:

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Developers Note (Optional)

Pre-launch Checklist (For PR creator)

As a creator of this PR:

  • ✍️ I have included clickup id and package/app_name in the PR title.
  • 👁️ I have gone through the code and removed any temporary changes (commented lines, prints, debug statements etc.).
  • ⚒️ I have fixed any errors/warnings shown by the analyzer/linter.
  • 📝 I have added documentation, comments and logging wherever required.
  • 🧪 I have added necessary tests for these changes.
  • 🔎 I have ensured all existing tests are passing.

Reviewers

@horam-deriv

Pre-launch Checklist (For Reviewers)

As a reviewer I ensure that:

  • ✴️ This PR follows the standard PR template.
  • 🪩 The information in this PR properly reflects the code changes.
  • 🧪 All the necessary tests for this PR's are passing.

Pre-launch Checklist (For QA)

  • 👌 It passes the acceptance criteria.

Pre-launch Checklist (For Maintainer)

  • [MAINTAINER_NAME] I make sure this PR fulfills its purpose.

@ilya-deriv ilya-deriv changed the title ilya/1561/Integration_of_Growthbook fix(deriv_feature_flag): ilya/1561/Integration_of_Growthbook Feb 6, 2024
@ilya-deriv ilya-deriv changed the title fix(deriv_feature_flag): ilya/1561/Integration_of_Growthbook fix(deriv_feature_flag): DERG-1561 Integration_of_Growthbook Feb 6, 2024
@ilya-deriv ilya-deriv changed the title fix(deriv_feature_flag): DERG-1561 Integration_of_Growthbook refactor(deriv_feature_flag): DERG-1561 Integration_of_Growthbook Feb 6, 2024
@horam-deriv
Copy link
Contributor

@ilya-deriv this is a wrong approach. This approach will make our client applications to be tightly coupled to our package.

@horam-deriv
Copy link
Contributor

@ilya-deriv i have a question from you. your key is inside your growthBook dashboard. In order to pass it to the package, you need to have it somewhere in runtime. How do you want to handle it?

@ilya-deriv
Copy link
Contributor Author

Hi @horam-deriv thanks for checking this.
Now the api_key is encrypted and added to .env file
However host_url is not added, so I need to somehow pass it.
Please advise me on what is a better approach here, thank you

@horam-deriv
Copy link
Contributor

Hi @horam-deriv thanks for checking this. Now the api_key is encrypted and added to .env file However host_url is not added, so I need to somehow pass it. Please advise me on what is a better approach here, thank you

Hi @ilya-deriv
Our app should not have direct dependency to none of our services for feature flag, logging, etc.
This is why I do not believe this approach is appropriate.
Furthermore, the url is constant for all of instances of DerivGo for all the users. what would the benefit of passing the url from the client application?
IMHO, the best approach is the one that makes our app not directly dependent to GrowthBook. So, I believe the best approach is the one which we have implemented right now.
For adding the url let me know if I can help.
I think @waqas-younas-deriv also has access for it.

@ilya-deriv
Copy link
Contributor Author

ilya-deriv commented Feb 12, 2024

@horam-deriv thank you, I got your point.
Would it be ok if I hardcode the url in deriv_feature_flag?

@ramin-deriv
Copy link
Contributor

Hi @horam-deriv thanks for checking this. Now the api_key is encrypted and added to .env file However host_url is not added, so I need to somehow pass it. Please advise me on what is a better approach here, thank you

Hi @ilya-deriv Our app should not have direct dependency to none of our services for feature flag, logging, etc. This is why I do not believe this approach is appropriate. Furthermore, the url is constant for all of instances of DerivGo for all the users. what would the benefit of passing the url from the client application? IMHO, the best approach is the one that makes our app not directly dependent to GrowthBook. So, I believe the best approach is the one which we have implemented right now. For adding the url let me know if I can help. I think @waqas-younas-deriv also has access for it.

@horam-deriv Making the feature flag package dependant to deriv_env, reduces the flexibility of the package.
We don't need to make DerivGO app be dependant to our feature flag service (GrowthBook), the abstraction and default implementation can be provided by the feature flag package:

abstract class FeatureFlagService {
}
class DerivGrowthBook implements FeatureFlagService {
}
class GrowthBookConfig {
 final String url;
 final String apiKey;
...
}
class DerivFeatureFlag {
  /// Initilize the FeatureFlag service for the whole app.
  Future<void> initialize(FeatureFlagService) async {
    ...
  }
..

In DerivGO side:

DerivFeatureFlag().initialize(DerivGrowthBook(GrowthBookConfig(url, key)));

the feature flag provides the default implementation which is DerivGrowthBook.
it will be flexible like the client app can create different implementation implementing FeatureFlagService
DerivGrowthBook is also all provided by the feature flag package and DerivGO doesn't need to depend on GrowthBook directly.

@sahani-deriv
Copy link
Contributor

I agree with @ramin-deriv’s approach. Including flexibility issue, having deriv_env as dependency in growthbook package might need extra maintenance because if a change happen in deriv_env we have to change in both app and the package itself.

Hi @horam-deriv thanks for checking this. Now the api_key is encrypted and added to .env file However host_url is not added, so I need to somehow pass it. Please advise me on what is a better approach here, thank you

Hi @ilya-deriv Our app should not have direct dependency to none of our services for feature flag, logging, etc. This is why I do not believe this approach is appropriate. Furthermore, the url is constant for all of instances of DerivGo for all the users. what would the benefit of passing the url from the client application? IMHO, the best approach is the one that makes our app not directly dependent to GrowthBook. So, I believe the best approach is the one which we have implemented right now. For adding the url let me know if I can help. I think @waqas-younas-deriv also has access for it.

@horam-deriv Making the feature flag package dependant to deriv_env, reduces the flexibility of the package. We don't need to make DerivGO app be dependant to our feature flag service (GrowthBook), the abstraction and default implementation can be provided by the feature flag package:

abstract class FeatureFlagService {
}
class DerivGrowthBook implements FeatureFlagService {
}
class GrowthBookConfig {
 final String url;
 final String apiKey;
...
}
class DerivFeatureFlag {
  /// Initilize the FeatureFlag service for the whole app.
  Future<void> initialize(FeatureFlagService) async {
    ...
  }
..

In DerivGO side:

DerivFeatureFlag().initialize(DerivGrowthBook(GrowthBookConfig(url, key)));

the feature flag provides the default implementation which is DerivGrowthBook. it will be flexible like the client app can create different implementation implementing FeatureFlagService DerivGrowthBook is also all provided by the feature flag package and DerivGO doesn't need to depend on GrowthBook directly.

@ahrar-deriv
Copy link
Contributor

this pr has been handled in #477

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