Skip to content

Conversation

@ZeynelKoca
Copy link

@ZeynelKoca ZeynelKoca commented Oct 6, 2025

Description

Adds a way to globally configure the state TTL for dynamo (inspired by #1059).

The defined default TTL will only be used if the state request does not explicitly specify a TTL itself.

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@JoostPV
Copy link

JoostPV commented Oct 16, 2025

Would be nice when the different statestores (Redis and Dynamo) behave in a similar way.

@MyMirelHub
Copy link
Contributor

There will be a slight delay in reviewing the DynamoDB-related PRs. We need to address the missing/disabled tests first, which are being tracked in issue #4047. Thanks for your patience!

@ZeynelKoca
Copy link
Author

@sicoyle Ready for another review round

Copy link
Contributor

@sicoyle sicoyle left a comment

Choose a reason for hiding this comment

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

one last comment and then this LGTM - thank you!

@ZeynelKoca
Copy link
Author

one last comment and then this LGTM - thank you!

Fixed. Also updated the docs PR with the same description

sicoyle
sicoyle previously approved these changes Oct 30, 2025
@sicoyle
Copy link
Contributor

sicoyle commented Oct 30, 2025

@ZeynelKoca pls see the build failures and ping when you're ready and I can retrigger 🙏

@ZeynelKoca
Copy link
Author

@sicoyle Fixed a stupid rebase mistake. Try again?

@ZeynelKoca
Copy link
Author

@sicoyle Can the pipeline run get an approval?

sicoyle
sicoyle previously approved these changes Nov 18, 2025
@ZeynelKoca
Copy link
Author

@sicoyle Fixed the formatting 🙏

@ZeynelKoca
Copy link
Author

@sicoyle I think a re-run is needed? Pipelines failed due to being unable to checkout the repo

@ZeynelKoca ZeynelKoca requested a review from sicoyle November 20, 2025 09:48
@sicoyle
Copy link
Contributor

sicoyle commented Nov 20, 2025

@sicoyle I think a re-run is needed? Pipelines failed due to being unable to checkout the repo

rerunning, lets see. I know github has been having quite a few issues recently

@ZeynelKoca
Copy link
Author

@sicoyle I think a re-run is needed? Pipelines failed due to being unable to checkout the repo

rerunning, lets see. I know github has been having quite a few issues recently

Fixed another formatting issue 🙏

@ZeynelKoca ZeynelKoca force-pushed the dynamo-global-ttl branch 2 times, most recently from 6b25c88 to 3eb43a9 Compare November 21, 2025 08:49
@ZeynelKoca
Copy link
Author

@sicoyle Can you trigger a pipeline run? I see that the corresponding docs PR has already been merged.

sicoyle
sicoyle previously approved these changes Dec 9, 2025
Copy link
Contributor

@sicoyle sicoyle left a comment

Choose a reason for hiding this comment

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

as long as the build passes, then LGTM. Thank you for your contribution and iterating on feedback 🤗

@ZeynelKoca ZeynelKoca force-pushed the dynamo-global-ttl branch 2 times, most recently from c4d17ec to 3713675 Compare December 9, 2025 23:21
@ZeynelKoca
Copy link
Author

ZeynelKoca commented Dec 9, 2025

@sicoyle Thanks! The build failed again due to formatting issues (🙄) so I upgraded to the exact go version the pipeline was using. go fmt now properly added the missing new-line at the end of the dynamodb_test.go file. Another pipeline run should do it 🤞

@ZeynelKoca ZeynelKoca requested a review from sicoyle December 9, 2025 23:25
@sicoyle
Copy link
Contributor

sicoyle commented Dec 10, 2025

The build failed again due to formatting issues (🙄)

LOL I feel this 😄 It's running. Will circle back later today and if all green we can send it 🚀

Signed-off-by: Zeynel Koca <[email protected]>
Signed-off-by: Zeynel Koca <[email protected]>
@ZeynelKoca
Copy link
Author

@sicoyle No formatting issues anymore 🎉

1 failing unit test though, which I updated based on the new TTL property behavior. All tests pass now locally

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.

4 participants