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: [M3-8837] - Add LKE-E feature flag #11259

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Nov 14, 2024

Description 📝

We will need a feature flag in Launch Darkly to gate access to the UI changes for LKE-Enterprise.

Changes 🔄

  • The lkeEnterprise flag exists in Launch Darkly.
    • The flag is EVERYTHING OFF in staging and prod environments and Feature ON, LA ON, GA OFF in the dev environment.
  • The flag is added to the dev tools feature flag list.
  • useIsLkeEnterpriseEnabled hook handles the enablement of the feature.
    • Since there is an account capability to enable the feature, the necessary account beta endpoint updates are included.
    • Test coverage was added for this hook.

How to test 🧪

Prerequisites

(How to setup test environment)

  • Have the LKE-E customer tag on your account (prod account is fine). See the internal LKE-Enterprise project tracker to find this.

Verification steps

(How to verify changes)

  • Read the diff since there are no UI changes yet.
  • Ensure that unit test coverage passes:
yarn test kubeUtils
  • Confirm that the LKE-E feature flag is present in dev tools.
  • If you want to test the hook in code, manually add the hook to a page like KubernetesLanding.tsx, console.log the return value, and confirm you see a v4beta/account network request.
  const { isLkeEnterpriseLAEnabled } = useIsLkeEnterpriseEnabled();
  console.log({ isLkeEnterpriseLAEnabled });

As an Author, I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@mjac0bs mjac0bs force-pushed the M3-8837-add-lke-enterprise-feature-flag branch from 9dd6ce7 to 53d3170 Compare November 14, 2024 22:12
Comment on lines -82 to -91
server.use(
http.get('*/account/betas/apl', () => {
return HttpResponse.json(accountBeta);
})
);
const { result } = renderHook(() => useAPLAvailability(), {
wrapper: (ui) => wrapWithTheme(ui, { flags: { apl: true } }),

queryMocks.useAccountBetaQuery.mockReturnValue({
data: accountBeta,
});
await waitFor(() => {
expect(result.current.showAPL).toBe(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.

I had to modify this test with the addition of the useIsLkeEnterpriseEnabled tests, due to the use of feature flags in both.

In the words of the wise @jdamore-linode, the APL test needed to be modified because:

the vi.mock for useFlags that technically broke the APL availability test, because the useAccountBetaQuery function calls useFlags. And since useAccountBetaQuery wasn’t getting mocked but useFlags was, useFlags was responding with an empty object causing useAccountBetaQuery to always return an object indicating that APL is disabled.

(And so the APL test would fail.)

In the interest of using one mocking solution in this file (not both server.use and vi.mock), I switched this one over to using the queryMocks, as well.

/**
* @TODO LKE-E - M3-8838: Clean up after released to GA, if not otherwise in use
*/
export const useAccountBeta = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not to be confused with the useAccountBetaQuery... if there's a suggestion for a better name here, I'm open to it!

@mjac0bs mjac0bs marked this pull request as ready for review November 15, 2024 17:06
@mjac0bs mjac0bs requested a review from a team as a code owner November 15, 2024 17:06
@mjac0bs mjac0bs requested review from carrillo-erik, pmakode-akamai and hana-akamai and removed request for a team November 15, 2024 17:06
Copy link

Coverage Report:
Base Coverage: 87.34%
Current Coverage: 87.34%

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 445 passing tests on test run #4 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing445 Passing2 Skipped79m 11s

@jdamore-linode
Copy link
Contributor

Have the LKE-E customer tag on your account (prod account is fine). See the internal LKE-Enterprise project tracker to find this.

Any thoughts about adding a Mock Service Worker preset for this? Doesn't help too much now since there aren't any UI changes, but might be handy once development ramps up a bit more.

I took a quick stab at a preset (that does the absolute bare minimum, there's probably a lot of room for improvement):

// New file at `src/mocks/presets/extra/account/lkeEnterpriseEnabled.ts` or similar

import { http } from 'msw';
import {
  accountFactory,
} from 'src/factories';
import { makeResponse } from 'src/mocks/utilities/response';

import type { MockPresetExtra } from 'src/mocks/types';

const mockLkeEnabledCapability = () => {
  return [
    http.get('*/v4*/account', async ({ request }) => {
      return makeResponse(
        accountFactory.build({
          capabilities: [
            // Other account capabilities might be necessary here, too...
            // TODO Make a `defaultAccountCapabilities` factory.
            'Kubernetes',
            'Kubernetes Enterprise',
          ],
        })
      );
    }),
  ]
};

export const lkeEnterpriseEnabledPreset: MockPresetExtra = {
  desc: 'Mock account with LKE Enterprise capability',
  group: { id: 'Account', type: 'select' },
  handlers: [mockLkeEnabledCapability],
  id: 'account:lke-enterprise-enabled',
  label: 'LKE Enterprise Enabled',
};

(And then there are some accompanying changes required to the MockPresetExtraId type in src/mocks/types.ts, and the preset needs to be added to the extraMockPresets array in src/mocks/presets/index.ts to get it to show up in the dev tools)

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Thanks @mjac0bs!

@mjac0bs
Copy link
Contributor Author

mjac0bs commented Nov 15, 2024

I'm going to stick this back in Draft for now after talking about RQ and cache management with Banks. (relevant comments here and here)

(Thanks for your comments, Joe. MSW preset is a good idea.)

@mjac0bs mjac0bs marked this pull request as draft November 15, 2024 19:32
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.

3 participants