Adds logic to the reconcile function to handle the optional infinite requeue (Issue #826) #23
Conversation
ack-bot
added
the
needs-ok-to-test
|
Hi @mbaijal. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
mbaijal
changed the title
|
/approve |
|
@mbaijal, There are merge conflicts. Can you please fix the conflicts, amend and force push again ? |
Yes, this is new over the weekend, let me fix it. |
mbaijal
force-pushed
the
infinite-requeue
branch
from
778f657 to
d5fad31
Compare
mbaijal
force-pushed
the
infinite-requeue
branch
from
d5fad31 to
2770256
Compare
mbaijal
changed the title
| ) (AWSResourceManager, error) | ||
| // IsAdoptable returns true if the resource is able to be adopted | ||
| IsAdoptable() bool | ||
| // RequeueOnSuccessSeconds returns true if the resource should be requeued after specified seconds |
There was a problem hiding this comment.
godoc is out of date. Please fix it and everything else looks very good. 💯
There was a problem hiding this comment.
@vijtrip2 I'll fix on a followup (handling the TODO above) :)
|
|
||
| // AWSResourceManagerFactory returns an AWSResourceManager that can be used to | ||
| // manage AWS resources for a particular AWS account | ||
| // TODO(jaypipes): Move AWSResourceManagerFactory into its own file |
| ) (AWSResourceManager, error) | ||
| // IsAdoptable returns true if the resource is able to be adopted | ||
| IsAdoptable() bool | ||
| // RequeueOnSuccessSeconds returns true if the resource should be requeued after specified seconds |
There was a problem hiding this comment.
@vijtrip2 I'll fix on a followup (handling the TODO above) :)
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jaypipes, mbaijal, 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 |
| rlog.Debug( | ||
| "requeueing resource after resource synced condition true", | ||
| ) | ||
| return requeue.NeededAfter(nil, time.Duration(duration)*time.Second) |
There was a problem hiding this comment.
@jaypipes isn't it better to pass the debug message on line #286 to requeue instead of nil?
There was a problem hiding this comment.
@surajkota the nil in the NeededAfter is for an error. There is no error here.
5 participants
This PR is one of two to implement Issue #826
Link to the code-generator PR
Testing
I tested the change by enabling it for applicationAutoscaling (ScalableTarget) - you can check the generated code on this branch just for reference.