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

support feature flag overrides via query string #2945

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

christianvogt
Copy link
Contributor

@christianvogt christianvogt commented Jun 21, 2024

https://issues.redhat.com/browse/RHOAIENG-7894

Description

Once feature flags are overridden in the current session a banner is displayed at the top of the page. From the banner the user can restore their session without the need for closing their browser window. They can also open a modal to more easily adjust the feature flags for the current session.
image

The modal lists all available feature flags and their current state. Flags that have been overridden in the session via query string or through this model are annotated as (overridden). If the current dashboard config doesn't contain a valid entry (likely only a dev issue with a mismatched frontend and backend), the checkbox will be in the partially checked state.
image

Changes to feature flags are immediately reflected in the UI.

While the modal is open, the query string is updated for the purpose of constructing a shareable URL that can also be bookmarked for future use.

Restoring defaults will immediately remove the banner.

To enable this feature, set the query string param to ?devFeatureFlags.
To adjust the feature flags via the query string, assign a value: ?devFeatureFlags=home,disableInfo,disableAppLauncher=false,enablement=true
Note that the query string accepts both variants of a flag which has the disable prefix or not. eg. disableHome=true is the same as home=false, or disableHome=false is the same as home or home=true

Some backend end points such as /api/cluster-settings reads from the dashboard config cached on the backend. In order to get around this caching, if feature flags are overridden in the front end, a new header is sent along with all requests. See x-odh-feature-flags.

The new header is supplied by setting the default headers for axios. As such all usage of axios must use our new singleton instance from ~/utilities/axios. To ensure compliance a new eslint rule is provided that errors if the node_modules axios instance is imported.

How Has This Been Tested?

Open any page and then change the query string to be ?devFeatureFlags=... where ... is either empty or a feature flag of your choice. eg home

Play around with the banner and feature flag modal.

As an admin navigate to Settings => Cluster settings and inspect the network tab. Look for the /api/cluster-settings call and inspect the response such that the enablement matches the front end feature flag settings:

    "modelServingPlatformEnabled": {
        "kServe": true,
        "modelMesh": true
    }

Test Impact

Added unit tests.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jun 21, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 21, 2024
@christianvogt christianvogt changed the title [WIP] ]support feature flag overrides via query string [WIP] support feature flag overrides via query string Jun 21, 2024
@christianvogt christianvogt force-pushed the dev-flags branch 2 times, most recently from 51a1330 to b1ef771 Compare June 21, 2024 22:04
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 21, 2024
@christianvogt christianvogt force-pushed the dev-flags branch 2 times, most recently from 9cb95ae to c335bd2 Compare June 21, 2024 22:19
andrewballantyne

This comment was marked as resolved.

@christianvogt

This comment was marked as resolved.

@christianvogt christianvogt changed the title [WIP] support feature flag overrides via query string support feature flag overrides via query string Jun 25, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jun 25, 2024
@andrewballantyne

This comment was marked as resolved.

@christianvogt

This comment was marked as resolved.

@christianvogt
Copy link
Contributor Author

christianvogt commented Jun 25, 2024

@andrewballantyne Thanks for the review. Sorry but just I added an extra feature :)

While the modal is open, the query param is now visible and updated in real time. This gives the ability to more easily construct the URL, bookmark it and share it. While the modal is closed the query string param is once again removed to best represent a normal user experience.

Do you think we should also include a hide banner option so that we can utilize this feature for screenshots without the extra clutter? I can see this as useful and hides the banner until the page refreshes.
eg:
image

Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

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

LGTM, pulled locally and tested, this is really awesome stuff

Copy link
Contributor

openshift-ci bot commented Jun 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mturley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit fce1bf5 into opendatahub-io:main Jun 26, 2024
6 checks passed
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