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

Patch the correct observed state after a rm.ReadOne call #76

Merged
merged 1 commit into from
Jun 7, 2021

Conversation

a-hilaly
Copy link
Member

@a-hilaly a-hilaly commented Jun 2, 2021

Issue aws-controllers-k8s/community#807

For some controllers we noticed an incorrect reconciliation behaviour
when a custom hook code was returning a RequeueNeededAfter error.
More precisely resourceManager.ReadOne was not returning the correct
observed state when sdkFind returns with an error. Which was causing
the reconciler to patch to wrong state (desird instead observed).

This patch fixes this issue by passing the correct observed state to onError.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@a-hilaly a-hilaly requested a review from jaypipes June 2, 2021 14:07
@a-hilaly
Copy link
Member Author

a-hilaly commented Jun 2, 2021

i'm currently testing this fix with ecr and dynamodb controllers
/hold

@ack-bot ack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2021
@jaypipes
Copy link
Collaborator

jaypipes commented Jun 2, 2021

@a-hilaly you may want to update the PR/commit message to mention that RequeueNeededAfter is actually an error and is the reason why we're noticing some of this behaviour in some of the custom hook code.

@vijtrip2
Copy link
Contributor

vijtrip2 commented Jun 2, 2021

/approve

For some controllers we noticed an incorrect reconciliation behaviour
when a custom hook code was returning a `RequeueNeededAfter` error.

More precisely `resourceManager.ReadOne` was not returning the correct
observed state when `sdkFind` returns with an error. This bug was causing
 the reconciler to patch to wrong state (`desird` instead `observed`).
This patch fixes this issue by passing the correct observed state to
`onError`.
@a-hilaly a-hilaly force-pushed the issue807/code-gen branch from d182f73 to f9fcdb6 Compare June 2, 2021 15:53
@a-hilaly
Copy link
Member Author

a-hilaly commented Jun 2, 2021

@jaypipes Updated, PTAL again.

@a-hilaly
Copy link
Member Author

a-hilaly commented Jun 2, 2021

/unhold

@ack-bot ack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2021
Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

👍

@jaypipes
Copy link
Collaborator

jaypipes commented Jun 7, 2021

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Jun 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, vijtrip2

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:
  • OWNERS [A-Hilaly,jaypipes,vijtrip2]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-bot ack-bot merged commit 8d7e9fa into aws-controllers-k8s:main Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants