Skip to content

allow to enable PVC feature flag for user via ConfigCat #13341

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

Merged
merged 1 commit into from
Oct 4, 2022
Merged

Conversation

sagor999
Copy link
Contributor

@sagor999 sagor999 commented Sep 26, 2022

Description

Instead of having to enable feature flag per user via admin panel, this allows to enable it via ConfigCat instead, as well as will allow us to roll out this feature to more users via configCat.

Related Issue(s)

Fixes #12745

How to test

Open preview env
Add your user id to ConfigCat non production env
Open new workspace
In terminal: ls -la /.workspace/.gitpod/ and you should see pvc file in there, indicating this workspace is running using PVC feature.

Release Notes

none

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@sagor999 sagor999 requested a review from a team September 26, 2022 23:14
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Sep 26, 2022
if (
// ConfigCat already has feature flag named `persistent_volume_claim` that controls if we show
// the option to enable PVC for project prebuilds. So have to use a different name here.
await getExperimentsClientForBackend().getValueAsync("user_pvc", false, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

webapp team, when reviewing, can you double check that this is indeed the only place where I need to add this code?

Copy link
Member

@geropl geropl Sep 28, 2022

Choose a reason for hiding this comment

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

This change can actually go away, the other two are fine! 👍
But didn't find time to test, yet 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Are you sure? Since I originally tested with this change only and it worked as expected. So want to double check before I remove this code. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure?

Yes. That's the core of the PR from last week: persistent_volume_claim is a "workspace persistent" feature, so it should never be required to modify on start, only on creation.

This PR just changes the source from where we get the info from, so we should be able to simply follow all occurrences of user.featureFlags?.permanentWSFeatureFlags?.includes("persistent_volume_claim") and that check there (as you already did).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geropl ah, good to know. thank you! code removed. PTAL

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-12745.1 because the annotations in the pull request description changed
(with .werft/ from main)

@geropl geropl self-assigned this Sep 27, 2022
@geropl
Copy link
Member

geropl commented Sep 27, 2022

Starting review now...

geropl
geropl previously requested changes Sep 27, 2022
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

The places where we have to replace/OR the existing logic are these:

@sagor999
Copy link
Contributor Author

@geropl updated code. Hopefully I did it the right way, not sure if adding all those injects all over the place is the correct way of doing it though. 😅

@jenting
Copy link
Contributor

jenting commented Oct 3, 2022

@geropl PTAL

@sagor999 sagor999 requested a review from a team October 3, 2022 18:26
@sagor999 sagor999 removed the request for review from geropl October 4, 2022 19:24
@sagor999 sagor999 dismissed geropl’s stale review October 4, 2022 19:25

requested changes addressed

@roboquat roboquat merged commit b40cc92 into main Oct 4, 2022
@roboquat roboquat deleted the pavel/12745 branch October 4, 2022 19:25
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ConfigCat to throttle adoption of PVC
5 participants