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

fix: [UIE-8194] dbaas maintenance pending updates state should display when any pending updates are available #11387

Conversation

smans-akamai
Copy link
Contributor

@smans-akamai smans-akamai commented Dec 9, 2024

Description 📝

For DBaaS maintenance feature, the pending updates state should display when there are any pending updates available for a DB cluster. It should also disable the option to upgrade the version while those updates are still pending.

Changes 🔄

List any change(s) relevant to the reviewer.

  • Fix check for pending updates so it will check for any available pending updates instead of only ones with a defined deadline or planned_for property.
  • Update disabling behavior for the Upgrade Version button in the DBaaS settings maintenance field so that it will disable when there are pending updates
  • When there are pending updates to disable Upgrade Version. Have a "?" icon to the right of the temporarily disabled Upgrade Version button with tip text:
    "Upgrades are disabled while maintenance updates are in progress."

Target release date 🗓️

1/14/25

Preview 📷

Before After
maintenance-update-before maintenance-update-after-2

How to test 🧪

Prerequisites

  • Managed Databases
  • dbaasV2 feature flag set for GA
  • Default DB created e.g. postgresql 13

Reproduction steps

  • Having a pending update available where deadline and the planned_for property are both null
  • In settings, see that both the Maintenance updates does not show review state
  • In settings, see that Upgrade Version button is enabled even with pending updates.

Verification steps

  • Verify that Maintenance field in DBaaS settings displays pending updates display pending update review state when pending updates are available.
  • Verify that Upgrade Version is disabled even if there are available version updates.
  • Verify that ? icon appears next to disabled Upgrade Version button when there are pending updates available
Author Checklists

As an Author, to speed up the review process, I 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

@smans-akamai smans-akamai requested a review from a team as a code owner December 9, 2024 16:55
@smans-akamai smans-akamai requested review from abailly-akamai and harsh-akamai and removed request for a team December 9, 2024 16:55
@smans-akamai smans-akamai force-pushed the UIE-8194-dbaas-settings-maintenance-pending-update-fix branch from febef64 to 4c771ae Compare December 9, 2024 18:21
Copy link

github-actions bot commented Dec 9, 2024

Coverage Report:
Base Coverage: 86.84%
Current Coverage: 86.84%

@cpathipa cpathipa added the DBaaS Relates to Database as a Service label Dec 9, 2024
@cpathipa cpathipa requested review from cpathipa and removed request for harsh-akamai December 9, 2024 20:27
@smans-akamai smans-akamai force-pushed the UIE-8194-dbaas-settings-maintenance-pending-update-fix branch from 4c771ae to 5e34741 Compare December 10, 2024 17:29
…y when any pending updates are available and disable version upgrade
@smans-akamai smans-akamai force-pushed the UIE-8194-dbaas-settings-maintenance-pending-update-fix branch from 5e34741 to a28bb9f Compare December 10, 2024 18:37
onClick={onUpgradeVersion}
>
Upgrade Version
</StyledLinkButton>
{hasUpdates && (
Copy link
Contributor Author

@smans-akamai smans-akamai Dec 10, 2024

Choose a reason for hiding this comment

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

@cpathipa The update was to have a Tooltip icon display when there are pending updates disabling the Upgrade Version button. Just pushed up another change to adjust the condition and I've updated the description.

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 1 failing test on test run #4 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
1 Failing465 Passing2 Skipped104m 15s

Details

Failing Tests
SpecTest
smoke-billing-activity.spec.tsBilling Activity Feed » displays correct timezone for invoice and payment dates

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/billing/smoke-billing-activity.spec.ts"

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.

Thanks for the contribution @smans-akamai - code looks good

Could we maybe use a server handler (or updating the factory if already handled) to mock a pending update to easily verify the change for now and future proofing? I was unable to verify the change since there's no pending update for the DB instances I am creating.

Boolean(
pendingUpdates?.some((update) => update.deadline || update.planned_for)
);
Boolean(pendingUpdates && pendingUpdates?.length > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Boolean(pendingUpdates && pendingUpdates?.length > 0);
Boolean(pendingUpdates?.length > 0);

Copy link
Contributor Author

@smans-akamai smans-akamai Dec 11, 2024

Choose a reason for hiding this comment

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

@abailly-akamai The pendingUpdates param is optional and can be undefined. So this change throws a "possibly undefined" typescript error without the added check. I believe the function was intentionally written this way so I'd prefer to leave it the as is.

Also, for the components using this method, the property that gets provided for pendingUpdates is optional. Perhaps because the data might not be available right away before making this check.

Since this just a fix for the existing behavior, can we leave it as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure thing, I just assumed the optional chaining was taking care of that 👍

@smans-akamai
Copy link
Contributor Author

smans-akamai commented Dec 11, 2024

Thanks for the contribution @smans-akamai - code looks good

Could we maybe use a server handler (or updating the factory if already handled) to mock a pending update to easily verify the change for now and future proofing? I was unable to verify the change since there's no pending update for the DB instances I am creating.

Thanks @abailly-akamai! The pending update is already mocked in the ServerHandler. Pending updates are included in the DB factory by default. This was done when the feature was originally implemented.
https://github.com/linode/manager/blob/develop/packages/manager/src/mocks/serverHandlers.ts#L269

If you change the pending updates array in the factory to an empty array, it should toggle this behavior in your local.
https://github.com/linode/manager/blob/develop/packages/manager/src/factories/databases.ts#L224

@abailly-akamai
Copy link
Contributor

abailly-akamai commented Dec 11, 2024

@smans-akamai that worked - It just won't do that with every cluster ID so I did not see it with the one I tried

for reference, /databases/mysql/19/settings while mocking with legacy server handlers will work 👍

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

✅ Changes look good - thanks @alban for the route to test 👍

@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Dec 12, 2024
@cpathipa cpathipa merged commit 15992df into linode:develop Dec 12, 2024
21 of 23 checks passed
Copy link

cypress bot commented Dec 12, 2024

Cloud Manager E2E    Run #6961

Run Properties:  status check passed Passed #6961  •  git commit 15992df8c7: fix: [UIE-8194] dbaas maintenance pending updates state should display when any ...
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6961
Run duration 28m 55s
Commit git commit 15992df8c7: fix: [UIE-8194] dbaas maintenance pending updates state should display when any ...
Committer smans-akamai
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 469
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! DBaaS Relates to Database as a Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants