fix: ensure we mark resource as adopted adopt-or-create #186
Conversation
|
/test s3-controller-test |
michaelhtm
force-pushed
the
fix/markasadoptedonupdate
branch
from
bf546cd to
0121d63
Compare
|
/test s3-controller-test |
michaelhtm
force-pushed
the
fix/markasadoptedonupdate
branch
from
0121d63 to
827a622
Compare
Currently the `adopted` annotation was getting removed when the resource goes through update path during adopt-or-create adoption. These changes ensure the annotation is persisted in k8s.
michaelhtm
force-pushed
the
fix/markasadoptedonupdate
branch
from
827a622 to
29515da
Compare
|
/test ecr-controller-test |
|
/test ec2-controller-test |
| _, err := r.Sync(ctx, rm, desired) | ||
| require.Nil(err) | ||
| rm.AssertNumberOfCalls(t, "ReadOne", 1) | ||
| rm.AssertCalled(t, "Update", ctx, desired, latest, delta) | ||
| rd.AssertCalled(t, "Delta", desired, latest) | ||
| rd.AssertNumberOfCalls(t, "MarkAdopted", 2) |
There was a problem hiding this comment.
Can we assert that the AwsResource has the expected adoption fields as well when it is patched? Checking that the function call happened twice is a good indication, but we could be more definitive.
There was a problem hiding this comment.
When MarkAdopted is called (an interface method) we're not really doing anything in these unit tests. We can only trust the actual implementation is handling the MarkAdopted is correct. The best way to actually test this is using e2e tests
There was a problem hiding this comment.
Ahh I see. That's due to the implementation being generated per service controller?
|
/test ec2-controller-test |
|
/lgtm Just holding in case we want a second reviewer. |
ack-prow
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knottnt, michaelhtm 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 |
|
/unhold |
ack-prow
bot
removed
the
do-not-merge/hold
|
/retest |
1 similar comment
|
/retest |
|
/test sagemaker-controller-test |
|
/test sagemaker-controller-test |
|
/test ec2-controller-test |
|
@michaelhtm: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/test ec2-controller-test |
Issue #2459#issuecomment-2879691276
Description of changes:
Currently the
adoptedannotation was getting removed when the resourcegoes through update path during adopt-or-create adoption. These changes
ensure the annotation is persisted in k8s.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.