MemoryDB Cluster resource support #17
Conversation
nmvk
force-pushed
the
mdbc
branch
3 times, most recently
from
ed8959f to
07fbcc0
Compare
There was a problem hiding this comment.
I haven't taken a chance to look at the tests yet, but really happy with the hook code you've added. It seems like there is a lot of complexity in these APIs and you've done a good job of separating it out into manageable chunks. I've left some comments inline regarding common code practices and some touchups. I'll take a look at tests and a little further into how we could re-use some of this code on the next review.
Support for MDB cluster resource. - Ensure that we invoke update API if there delta - Scale up before scale in, Scale out before scale down - Use annotation to rollback in case of async failures - E2E tests to cover Create/Update/Rollback - TerminalConditions for User errors - Wait for delete to be completed - Perform engineversion upgrade and scale up/down whenever possible
Additionally used new code-gen and runtime.
There was a problem hiding this comment.
Just a few small nits and then I'm happy.
I do have a bit of a problem with these declarative tests in that we are only checking the fields of the K8s resource. We never describe the state of the actual MemoryDB cluster. It feels like it's kind of defeating the purpose of an e2e test, if we don't test what we're integrating with. This probably needs to be figured out before we can push declarative tests through to all controllers
Fix constants, error message, and reque upon error.
Use latest version
|
Thank you! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nmvk, RedbackThomson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Support for MDB cluster resource.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.