Skip to content
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

Add retries and retryTimeout option #20

Merged
merged 16 commits into from
Dec 20, 2020

Conversation

tjenkinson
Copy link
Contributor

I haven't tested this yet (but not really sure how), and there aren't any unit tests either. Let me know if you'd like unit tests and I'll add some.

This adds retries, where the delay gets larger each time, and a retry is not scheduled if more than retryTimeout seconds has passed. I'm hoping this will be useful when the concurrency limit is reached. When that happens I'm assuming the ready file isn't created?

@tjenkinson tjenkinson changed the title Retries Add Retries Dec 4, 2020
@tjenkinson tjenkinson changed the title Add Retries Add retries and retryTimeout option Dec 4, 2020
@@ -1,5 +1,4 @@
Sauce Connect Proxy GitHub Action
=================================
# Sauce Connect Proxy GitHub Action
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vscode auto reformatted this file. Can revert the style changes if you like

@tjenkinson
Copy link
Contributor Author

Hmm unfortunately it looks when the limit is reached the file is still written :/

Maybe we’ll have to check the output? Or maybe the ready file contains something useful?

Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

some comments

src/main.ts Show resolved Hide resolved
src/start.ts Outdated Show resolved Hide resolved
saveState('containerId', containerId)
return
} catch (e) {
if (Date.now() - startTime >= retryTimeout) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This means by default there will be no retries, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I thought we should keep the existing behaviour the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we should enable retries by default. We expect the tunnel to start with the first try so we wouldn't modify the existing behavior anyway. And having the action to retry rather than fail seems to me like an desired feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok cool. Do you think 10 minutes would be a good default?

@tjenkinson
Copy link
Contributor Author

I'm thinking in order to catch the concurrent limit error (and possibly others) I might add a connection check step after wait() has resolved.

@tjenkinson
Copy link
Contributor Author

Hmm unfortunately it looks when the limit is reached the file is still written :/

I was using the wrong version of the action 🤦 . Done some testing and the file isn't written, so it works properly :)

because the log says it can effect performance when enabled
Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

👍 thanks!

@christian-bromann christian-bromann merged commit ddfd51e into saucelabs:master Dec 20, 2020
@tjenkinson
Copy link
Contributor Author

@christian-bromann please can you update the dist folder so we can switch back this repo?

@christian-bromann
Copy link
Contributor

@tjenkinson what do you mean by that?

@tjenkinson
Copy link
Contributor Author

I mean this folder https://github.com/saucelabs/sauce-connect-action/tree/master/dist

I didn't run the build as part of the PR so that folder on master is out of date.

@christian-bromann
Copy link
Contributor

Done. Released as v1.1.2

@tjenkinson
Copy link
Contributor Author

Awesome thanks!

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.

None yet

2 participants