-
Notifications
You must be signed in to change notification settings - Fork 354
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-8216] - NodeBalancer Query Key Factory #10556
refactor: [M3-8216] - NodeBalancer Query Key Factory #10556
Conversation
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.
Overall LGTM!
- Verified NodeBalancer functionality
- Verified assigning / unassigning NodeBalancers from Firewalls
Coverage Report: ❌ |
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.
Widespread NodeBalancer PRs always make me a little nervous because our e2e coverage is lacking for the feature. We really only test the basics of the create flow and empty state landing. It would be really nice to have test coverage for the landing page and details pages, but I think those exist somewhere in the backlog.
Spent some time testing functionality, including NBFW CRUD interactions, and things were looking good for the most part. See my comment about the issue with the getAllNodeBalancerConfigs
query that was causing regressions.
I think I got all of that addressed! Thanks for catching those ✏️ @mjac0bs |
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.
Thanks @bnussman-akamai, all looks good now! 🚢
Description 📝
Preview 📷
Note
No UI changes expected
How to test 🧪
As an Author I have considered 🤔