- 
                Notifications
    You must be signed in to change notification settings 
- Fork 180
handle requeues after setting Synced condition #73
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
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.
Sweet! Left few comments below
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 overall. Great testing @vijtrip2
/approve
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.
@vijtrip2 please separate this into two PRs, one for the code changes to call IsSynced in ensureConditions and another for the resource reference resolution bits. It's difficult to review this PR as it stands because these two code changes are unrelated to each other.
This reverts commit a7fc336.
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.
Just one question, otherwise good to go
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.
Rock on, thanks for separating the PRs :)
| /lgtm | 
| [APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, jaypipes, RedbackThomson, 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:
 
 Approvers can indicate their approval by writing  | 
Issue: aws-controllers-k8s/community#1186
Description of changes:
handleRequeuesmethod after setting theACK.ResourceSyncedconditionAWSResourceManager.IsSyncedmethod to determine theACK.ResourceSyncedconditionACK.ResourceSyncedcondition insideensureConditionsfunction when the condition is originally missingrm.IsSynced()call and Add a new unit-test whenrm.IsSyncedreturns falseBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.