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

feat: implement feature switch [WEB-535] #5310

Merged
merged 3 commits into from
Oct 24, 2022

Conversation

gt2345
Copy link
Contributor

@gt2345 gt2345 commented Oct 20, 2022

Description

Add feature_switch to master config, and every valid switch in the list is considered “on” for feature switching at front end.

Test Plan

Add feature_switch to master config
Verify that /master return with featureSwitch

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@gt2345 gt2345 requested a review from ioga as a code owner October 20, 2022 22:05
@cla-bot cla-bot bot added the cla-signed label Oct 20, 2022
@netlify
Copy link

netlify bot commented Oct 20, 2022

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 028304d
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6356dcec97ea8100095728ce
😎 Deploy Preview https://deploy-preview-5310--determined-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Oct 20, 2022

Deploy Preview for storybook-det ready!

Name Link
🔨 Latest commit 028304d
🔍 Latest deploy log https://app.netlify.com/sites/storybook-det/deploys/6356dcec9605d40008f9b8c2
😎 Deploy Preview https://deploy-preview-5310--storybook-det.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.


return (
<ul>
<li>{feature.isOn('trials_comparison') && 'trials_comparison'}</li>
Copy link
Contributor

@ClaireNeveu ClaireNeveu Oct 21, 2022

Choose a reason for hiding this comment

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

So that this isn't reliant on the current set of feature switches you should do a cast:

const feature1 = 'feature1' as ValidFeature
const feature2 = 'feature2' as ValidFeature

Then you can test if feature1 is on and feature2 is off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great!

@@ -50,6 +50,8 @@ message GetMasterResponse {
string branding = 9;
// Feature flag for RBAC and user groups.
bool rbac_enabled = 10;
// List of features that is on.
repeated string feature_switches = 11;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we please tag it as = 12? otherwise it'll conflict with this SaaS change we are yet to move to OSS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem

Copy link
Contributor

@ClaireNeveu ClaireNeveu left a comment

Choose a reason for hiding this comment

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

This looks good to me. Please wait for approval from @ioga before merging.

Copy link
Contributor

@ioga ioga left a comment

Choose a reason for hiding this comment

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

lgtm

@gt2345 gt2345 merged commit 45d30bc into determined-ai:master Oct 24, 2022
@dannysauer dannysauer added this to the 0.19.7 milestone Feb 6, 2024
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.

4 participants