Skip to content

Conversation

MaienM
Copy link

@MaienM MaienM commented Feb 8, 2023

I find that for many of my scripts runtime errors are not recoverable (especially during development where they might just be caused by something simple like invalid syntax/paths/etc), and retrying until the timeout is hit is really just a waste of time. To avoid this I've added a retry limit so you can tell the provider to just give up after x attempts.

I might be misunderstanding how the resource versioning/SchemaVersion system works, but as far as I could tell v3 of the resource did not introduce any actual changes, nor did it actually bump the SchemaVersion, so I just put my changes in that version and bumped the version. Please let me know if this is incorrect.

@loafoe
Copy link
Owner

loafoe commented Feb 8, 2023

@MaienM very timely PR as I hit this exact issue myself today. The existing retry mechanism is indeed quite naive. Ideally retry should only be done on connection errors or other network related trouble and not on command failure. I'll have a look shortly. Thanks for raising this!

@loafoe
Copy link
Owner

loafoe commented Feb 10, 2023

@MaienM there was a recent regression in the provider which caused timeout values to be totally out of whack, can you try 2.6.0 and see if it works better for you? Meantime I will try a slightly different approach to retry shortly

@MaienM
Copy link
Author

MaienM commented Apr 28, 2023

I've tried 2.6.0 and it doesn't seem substantially different from 2.5.0 to me. I'm not sure I ever noticed the issue with the timeouts to begin with so I suppose that's not too surprising. This may be due to the timeout on this command being set high enough (as it might need it in some cases) that I don't think I've ever actually hit it.

Have you had a chance to look into the different approach to retry that you mentioned previously? I'm not set on things being implemented the way I did in this PR, but using 2.6.0 I still very much miss a way to say "don't retry this command, if it fails just treat that as that resource failing to apply".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants