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

refactor: [M3-8064] - Clean up Database Feature Flagging Logic #10435

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented May 2, 2024

Description 📝

  • Gracefully migrate databases off of isFeatureEnabled 👋
  • Clean up duplicate logic for Database conditional rendering 🎏
  • Document why databases are handled in a weird way ✍️
  • Add unit testing 🧪

Preview 📷

Note

No UI changes expected

How to test 🧪

Prerequisites

Important

Please test as an unrestricted user and a restricted user

  • Verify Databases show for unrestricted users (dbaas-eos-opt-in customer tag needed)
  • Verify Databases show for restricted users

Note

To gracefully migrate off of isFeatureEnabled, we no longer check for flags.databases. The logic will be completely based on the API's customer capabilities. After we release this, we can turn flags.databases on in production and come back and use isFeatureEnabledV2 with the flag

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

@bnussman-akamai bnussman-akamai added the DBaaS Relates to Database as a Service label May 2, 2024
@bnussman-akamai bnussman-akamai self-assigned this May 2, 2024
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner May 2, 2024 16:48
@bnussman-akamai bnussman-akamai requested review from mjac0bs and abailly-akamai and removed request for a team May 2, 2024 16:48
Copy link

github-actions bot commented May 2, 2024

Coverage Report:
Base Coverage: 82.01%
Current Coverage: 82%

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Do we need to do the same for the GoTo menu? it's an edge case but while we're at it...

* (which is restricted users with no billing access),
* we must check if they can load Database Engines as a workaround.
* If these users can successfully fetch database engines, we will
* show databases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why we'd want to show dbaas for restricted users with no billing access?

This PR just moves the fetching to this util (so I don't think we're fetching more than before) but it would be nice to clarify cause I don't think that's something we do for other entities?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to improve the clarification. In summary, Databases are end of sale so they only should show for users who we know for sure have the account capability. The restricted user case is kind of wacky because a restricted user may or may not have access to GET /v4/account

Added Databases to the GoTo menu ✅

packages/manager/src/features/Databases/utilities.ts Outdated Show resolved Hide resolved
export const useIsDatabasesEnabled = () => {
const { data: account } = useAccount();

// If the flag is off AND we don't have permission to GET /v4/account,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is specifying whether the flag is off relevant anymore? We could probably omit those words now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, removed ✅

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Confirmed:

  • Databases are visible for an unrestricted user with the API capability
  • Databases are visible for a restricted user without access to /account the API capability
  • Without the capability, Databases is not visible in the nav, top menu, etc.

It looks like you have a merge conflict to resolve, but otherwise LGTM. Nits for typos.

packages/manager/src/features/Databases/utilities.ts Outdated Show resolved Hide resolved
packages/manager/src/features/Databases/utilities.ts Outdated Show resolved Hide resolved
@bnussman-akamai bnussman-akamai merged commit 37586d4 into linode:develop May 7, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DBaaS Relates to Database as a Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants