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

IS-577: Move site privatisation to growth book #1529

Merged
merged 7 commits into from
Sep 29, 2023

Conversation

harishv7
Copy link
Contributor

Problem

Site privatisation is feature-flagged via env var now with just a boolean. We want to move this to growthbook and to allow site-level whitelist.

Closes IS-577

Solution

Moved to growthbook with new feature flag and json response of all the whitelisted sites for repo privatisation feature.

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Tests

  • Unit tests (using npm run tests)
  • e2e tests (comment on this PR with the text !run e2e)
  • Smoke tests
    • On growthbook, whitelist your test-repo for the dev stage only or if on staging, for staging only
    • On CMS, visit site settings and ensure you see the Privacy Settings option

Deploy Notes

Removed environment variables:

  • REACT_APP_IS_SITE_PRIVATISATION_ACTIVE : removed in favour of new feature flag on GB

@harishv7 harishv7 requested review from alexanderleegs, seaerchin, kishore03109 and dcshzj and removed request for alexanderleegs and seaerchin September 27, 2023 02:35
@harishv7 harishv7 temporarily deployed to staging September 27, 2023 02:44 — with GitHub Actions Inactive
@harishv7 harishv7 temporarily deployed to staging September 27, 2023 02:49 — with GitHub Actions Inactive
// Example usage: const gb = useGrowthBook<FeatureFlags>();
export interface FeatureFlags {
[FEATURE_FLAGS.STYLING_REVAMP]: boolean
[FEATURE_FLAGS.REPO_PRIVATISATION]: PrivatisationWhitelist
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm why are we not using a boolean value like styling revamp? This way we are exposing info to the client on which sites use this feature, which seems something that we can avoid by switching to boolean type instead, would this be feasible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to gate the rollout slowly. See Slack convo for more info

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm correct me if I am wrong ya but we can still rollout per site basis without returing the entire list to the client right? eg here:
Screenshot 2023-09-27 at 5 27 08 PM

Copy link
Contributor Author

@harishv7 harishv7 Sep 28, 2023

Choose a reason for hiding this comment

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

Oh that's what you meant! I wanted to be explicit, but actually you have a very good point. We can off-load the conditional part to growthbook and client side doesn't need to know about the whitelist at all! Let me try this approach 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

growthbook?.getFeatureValue(
FEATURE_FLAGS.REPO_PRIVATISATION,
fallbackWhitelist
) || fallbackWhitelist
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm abit a bit confused here, why would growthbook be undefined ah?

  • how do we reason about fallbackWhiteList? if this list does not contain anything, should this still live in production line ya?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Return of useGrowthbook is this:

 useGrowthBook<FeatureFlags>(): GrowthBook<FeatureFlags> | undefined

It can be undefined because under the hood it uses a react context and growthbook if not yet initialised will return undefined

The fallbackWhitelist helps in 2 scenarios:

  1. If the useGrowthbook call returns undefined
  2. If growthbook is initialised but the fetching from growthbook fails

Hence, it should be there for defensiveness

Copy link
Contributor

Choose a reason for hiding this comment

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

So moving forward we are going to maintain 2 sot arh? more in favour of just || [], and we expect growthbook to work most of the time, but if it doesnt we have a sane value to prevent app from crashing rather than maintaining 2 sot!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, how did you get 2 sot? There is still 1 sot on growthbook. The fallback value here is already the mechanism for preventing the app from crashing

@harishv7 harishv7 temporarily deployed to staging September 28, 2023 05:21 — with GitHub Actions Inactive
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

lgtm w nit

src/hooks/settingsHooks/useGetSettings.ts Outdated Show resolved Hide resolved
@harishv7 harishv7 temporarily deployed to staging September 28, 2023 08:23 — with GitHub Actions Inactive
@harishv7 harishv7 merged commit 7b3804b into develop Sep 29, 2023
@mergify mergify bot deleted the IS-577-move-site-privatisation-to-growth-book branch September 29, 2023 04:55
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.

2 participants