-
Notifications
You must be signed in to change notification settings - Fork 12
add retries for ephemeral instance api calls #61
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
base: main
Are you sure you want to change the base?
Conversation
|
The ephemeral instance for the application preview has been shut down |
2224b31 to
3ba8a36
Compare
| pull_request: | ||
| paths-ignore: | ||
| - ./*.md | ||
| - LICENSE |
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.
removing the workflow_dispatch that I added in a previous PR, as it doesn't make sense with the logic that the ephemeral instances are currently following (e.g. searching for the PR number and adding comment to the PR)
| description: 'LocalStack API key used to access the platform api' | ||
| required: true | ||
| description: 'LocalStack Auth Token used to access the platform api' | ||
| required: false |
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.
the localstack-api-key is not documented, neither is it required or currently used in our own CI test. It is a fallback in case the LOCALSTACK_AUTH_TOKEN and LOCALSTACK_API_KEY are not set
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.
Thanks, that's a good point! That's maybe something to revisit in the future. The term is outdated (we only have auth tokens now) and it seems a bit confusing to configure...
alexrashed
left a comment
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.
Thanks a lot for fixing the usage of ephemeral instances by implementing the retry logic!
From my perspective this is looking great. I think there is potential to move this to the CLI and reuse it here in the future, but that would definitely be out of scope for this PR!
Given that @lukqw is the expert on ephemeral instances and also has contributed to this feature in the action in the past, I think it might be good to maybe also get a quick review from you as well? 🙏🏽
| description: 'LocalStack API key used to access the platform api' | ||
| required: true | ||
| description: 'LocalStack Auth Token used to access the platform api' | ||
| required: false |
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.
Thanks, that's a good point! That's maybe something to revisit in the future. The term is outdated (we only have auth tokens now) and it seems a bit confusing to configure...
| # retry() function: Retries a given command up to 'retries' times with a 'wait' interval. | ||
| # Usage: retry <command> | ||
| # Example: retry my_api_call_function | ||
| retry() { |
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.
@lukqw I think this is where your input would really be valuable. Is this retry mechanism what you would expect a client to do if they get a failure response from the ephemeral instance API?
This effectively would be a classic retry machanism, 5 retries, 5 seconds constant backoff.
As currently the API for ephemeral instances is known to be unstable, this PR aims to make to retry failing API calls and to capture additional logs in case of failure cases.
This way it will be easier to pinpoint issues in the future.
Example logs:
changes:
Suggested future work (once the API is stable):
auto-load-pod,ci-project