-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
VTAdmin: Add advanced workflow switchtraffic
options
#17658
base: main
Are you sure you want to change the base?
VTAdmin: Add advanced workflow switchtraffic
options
#17658
Conversation
Signed-off-by: Noble Mittal <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
switchtraffic
options
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17658 +/- ##
==========================================
- Coverage 67.76% 67.46% -0.31%
==========================================
Files 1585 1592 +7
Lines 255611 258158 +2547
==========================================
+ Hits 173217 174166 +949
- Misses 82394 83992 +1598 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! The only thing is that initializeTargetSequences isn't a boolean.
Beyond that, I tried to do various manual tests locally to confirm behavior. Do we not currently have any testing infrastructure in place to test the web UI? Something like Selenium, Puppeteer, Cypress, etc? The more we use VTAdmin for operations that can cause data loss and downtime, the more critical this testing becomes (in the past it was for the most part a read-only view of the cluster).
web/vtadmin/src/components/routes/workflows/WorkflowActions.tsx
Outdated
Show resolved
Hide resolved
@@ -72,7 +72,7 @@ export const Tooltip = ({ children, text }: TooltipProps) => { | |||
}); | |||
|
|||
return ( | |||
<Popover content={content} isOpen={isOpen}> | |||
<Popover content={content} isOpen={isOpen} containerStyle={{ zIndex: '20' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use tailwind styling here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't use tailwind for the containerStyle
prop as it expects a CSSStyleDeclaration
object, but I figured out it also has a prop called containerClassName
which can use tailwind classes. So, used that instead.
web/vtadmin/src/components/routes/workflows/WorkflowActions.tsx
Outdated
Show resolved
Hide resolved
web/vtadmin/src/components/routes/workflows/WorkflowActions.tsx
Outdated
Show resolved
Hide resolved
You can add some tests for this component like: https://github.com/vitessio/vitess/blob/main/web/vtadmin/src/components/ReadOnlyGate.test.tsx |
Signed-off-by: Noble Mittal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! @beingnoble03 were you able to look into the test framework that @notfelineit mentioned? Not required, but it would be nice to have.
I was waiting on the test before final review 😄 The code so far looks good to me though! |
@beingnoble03 @notfelineit yeah, working on it. will add it by tomorrow for sure. |
Signed-off-by: Noble Mittal <[email protected]>
pushed some unit tests for |
Description
This PR adds advanced workflow switch options in VTAdmin web. Also, a minor change:
Reshard
workflow can now be completed from VTAdmin itself.Screenshots
Related Issue(s)
Checklist
Deployment Notes