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 rebac feature flag via query params #1871

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ninfa-Jeon
Copy link
Contributor

@Ninfa-Jeon Ninfa-Jeon commented Feb 28, 2025

Done

This commit introduces query parameter-based feature flags to control the enabling and disabling of rebac

  • Added useFeatureFlags hook to manage enable-flag and disable-flag query parameters.
  • Implemented logic to determine flag status based on query parameters.
  • Updated relevant components to utilize the feature flags and adjust rebac behavior accordingly.

BREAKING CHANGE: The rebac logic is now controlled by query parameters. Existing behaviour may be affected if the query parameters are not set appropriately.

QA

  • Try accessing the dashboard via local controller
  • All should be fine
  • Login to dashboard via JIMM
  • Without any query params, it should hide rebac-related fields
  • Add query param enable-flag=rebac
  • Local storage should now show a flags key with value ["rebac"]
  • All rebac-related fields should appear
  • Add query param disable-flag=rebac
  • Local storage should now show a flags key with value []
  • All rebac-related fields should be hidden
  • Adding and removing multiple flags via query params should be supported

Details

https://warthogs.atlassian.net/browse/WD-18923

@webteam-app
Copy link

Copy link

github-actions bot commented Feb 28, 2025

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 97.89% (🎯 95%) 14421 / 14731
🟢 Statements 97.89% (🎯 95%) 14421 / 14731
🟢 Functions 98.2% (🎯 95%) 602 / 613
🟢 Branches 92.14% (🎯 90%) 3004 / 3260
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/consts.ts 100% 100% 100% 100%
src/components/LogIn/LogIn.tsx 100% 100% 100% 100%
src/hooks/useFeatureFlags.ts 100% 100% 100% 100%
src/store/middleware/model-poller.ts 96.02% 91.76% 100% 96.02% 55-56, 117-124, 159-160, 212, 410
Generated in workflow #188 for commit 32d9e8f by the Vitest Coverage Report Action

Copy link

@fasih-mehmood fasih-mehmood left a comment

Choose a reason for hiding this comment

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

Don't mind me, just passing through here 😄

I recently implemented localStorage based feature flags in Canonical Console so as soon as I read "feature flag" in the PR title, I became interested to see how other devs implement it.

Flags in query parameter is an interesting way to implement feature flagging. Definitely a lot easier to explain to end users then to have them open the dev tools and run a command to update the local storage xD

Great work!

@@ -293,6 +294,7 @@ describe("model poller", () => {
});

it("updates the controller features if JIMM >= 4", async () => {
localStorage.setItem("rebac", JSON.stringify("true"));

Choose a reason for hiding this comment

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

You don't have to use JSON.stringify here as "true" is already a string.

Suggested change
localStorage.setItem("rebac", JSON.stringify("true"));
localStorage.setItem("rebac", "true");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! We ended up changing the approach a bit but I will keep this in mind for future haha

@@ -26,6 +28,8 @@ import UserPassForm from "./UserPassForm";
import { ErrorResponse, Label, TestId } from "./types";

export default function LogIn() {
const { getEnabledFlag, getDisabledFlag } = useFeatureFlags();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to move this hook into the App component as it isn't related to authentication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow, I first implemented this in App component but it wouldn't work. Will retry.

Copy link
Contributor Author

@Ninfa-Jeon Ninfa-Jeon Mar 3, 2025

Choose a reason for hiding this comment

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

we cannot place the useFeatureFlags hook in App.tsx because it uses useQueryParam hook that in turn uses useSearchParams hook. These hooks can only be placed in a component that is wrapped in a router, which essentially, comes under the App.tsx
a different approach could be to use BrowserRouter from react-router-dom but that would require a lot of refactor

@Ninfa-Jeon
Copy link
Contributor Author

Don't mind me, just passing through here 😄

I recently implemented localStorage based feature flags in Canonical Console so as soon as I read "feature flag" in the PR title, I became interested to see how other devs implement it.

Flags in query parameter is an interesting way to implement feature flagging. Definitely a lot easier to explain to end users then to have them open the dev tools and run a command to update the local storage xD

Great work!

Thank you @fasih-mehmood ! The idea for this was Huw's so its all thanks to him haha.

@Ninfa-Jeon Ninfa-Jeon force-pushed the feature-flag-rebac branch from 06987dc to 32d9e8f Compare March 3, 2025 08:39
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