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.

Sign up for GitHub

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

Jump to bottom

576 Add methods to check SSH connections/commands with retry #705

Merged
merged 2 commits into from
Mar 31, 2021
Merged

576 Add methods to check SSH connections/commands with retry #705

merged 2 commits into from
Mar 31, 2021

Conversation

ztbrown
Copy link
Contributor

@ztbrown ztbrown commented Nov 11, 2020

Early PR for feedback on approach for addressing #576

@rhoboat, I'm working on the helper methods you suggested in 576 and I want to make sure this is an approach you are comfortable with.

I wrote CheckSshConnectionWithRetry and CheckSshConnectionWithRetryE to accept an optional function. They default to wrapping CheckSshConnectionE, but the option to inject means we can swap it for a mock function and unit test much easier. My worry is that the interface might not be as clear as it could be to users. What do you think?

If this looks good, I'll move on to CheckSshCommandWithRetry and CheckSshCommandWithRetryE. Thanks!

@brikis98
Copy link
Member

Just a heads up that @rhoboat is currently OOO, but they will take a look when they are back!

@ztbrown
Copy link
Contributor Author

ztbrown commented Dec 29, 2020

hey @rhoboat! Bumping this for visibility. I can keep going, make alterations or close the PR. Thanks!

@ztbrown
Copy link
Contributor Author

ztbrown commented Feb 9, 2021

Bump.

@brikis98
Copy link
Member

@rhoboat Could you take a look?

@ztbrown Could you pull in the latest from master?

@ztbrown ztbrown force-pushed the 576-add-method-to-check-ssh-connections branch from 88bc612 to 28138f6 Compare February 26, 2021 15:54
@ztbrown
Copy link
Contributor Author

ztbrown commented Feb 26, 2021

rebased against master. This is still what I would consider a WIP or early PR, looking for feedback. Do we like this approach? Is it too clever by half? If we don't like it, how would you like to proceed?

Thanks!

@ztbrown
Copy link
Contributor Author

ztbrown commented Mar 29, 2021

Bump. If you all would like me to drop it, I will close. LMK!

@rhoboat
Copy link
Contributor

rhoboat commented Mar 30, 2021

This looks good! I'm taking a closer look now...

@rhoboat
Copy link
Contributor

rhoboat commented Mar 30, 2021

I think you're totally on the right track @ztbrown. Keep going, and feel free to ping me again when you're ready for a final review.

@ztbrown
Copy link
Contributor Author

ztbrown commented Mar 30, 2021

That rocks! Thank you. I'll set aside some time to jump back in. Thanks again!

@rhoboat
Copy link
Contributor

rhoboat commented Mar 30, 2021

Great! Also I wanted to apologize for missing your work on this months ago. 😞

@ztbrown
Copy link
Contributor Author

ztbrown commented Mar 30, 2021

Thank you but it's totally understandable. :)

@ztbrown ztbrown force-pushed the 576-add-method-to-check-ssh-connections branch 5 times, most recently from fe9607a to 2c5b9a5 Compare March 30, 2021 23:50
@ztbrown ztbrown changed the title [WIP] 576 Add methods to check SSH connections/commands with retry 576 Add methods to check SSH connections/commands with retry Mar 30, 2021
@ztbrown ztbrown force-pushed the 576-add-method-to-check-ssh-connections branch from 2c5b9a5 to d9f6161 Compare March 30, 2021 23:53
@ztbrown
Copy link
Contributor Author

ztbrown commented Mar 30, 2021

OK @rhoboat, looking for a final review. I've added the retry logic to CheckSshCommandWithRetry and CheckSshCommandWithRetryE. Added some tests for when retries get exceeded on CheckSshCommandWithRetryE and CheckSshConnectionWithRetryE. Let me know what you think. Thanks!

@rhoboat
Copy link
Contributor

rhoboat commented Mar 31, 2021 •
edited
Loading

It's odd that the checks for this PR are not running, i.e., tests. I'd like to see them succeed on CI. (Looking into this)

@infraredgirl
Copy link
Contributor

I just kicked off the tests.

@ztbrown
Copy link
Contributor Author

ztbrown commented Mar 31, 2021

lmk if you want me to rebase against master, squash down to one commit or just press the "resolve conflicts" button above.

@yorinasub17
Copy link
Contributor

@ztbrown Would be best if you can rebase. Since it's a go.sum conflict, this is typically better to resolve by wiping out the file and running go mod tidy to ensure all the dependencies resolve correctly.

@ztbrown
Copy link
Contributor Author

ztbrown commented Mar 31, 2021

awesome. I'm on it.

ztbrown and others added 2 commits March 31, 2021 12:59
@ztbrown
CheckSshConnectionWithRetry / CheckSshConnectionWithRetryE + tests
421f248
@ztbrown
Add methods for testing SSH Command
061deaa 
* CheckSshCommandEWithRetry
* CheckSshCommandWithRetry
* Tests for failing on max retries
* Tests for passing after some retries
@ztbrown ztbrown force-pushed the 576-add-method-to-check-ssh-connections branch from d9f6161 to 061deaa Compare March 31, 2021 16:59
@ztbrown
Copy link
Contributor Author

ztbrown commented Mar 31, 2021

@yorinasub17 done!

@yorinasub17
Copy link
Contributor

Thanks! Just kicked off another build.

yorinasub17
yorinasub17 approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

LGTM! And the test failure is unrelated to this change, so will move forward with merging this in! Thanks for your patience getting this over the finish line!

@yorinasub17 yorinasub17 merged commit 2e4cdf8 into gruntwork-io:master Mar 31, 2021
@ztbrown ztbrown deleted the 576-add-method-to-check-ssh-connections branch March 31, 2021 20:00
@ztbrown
Copy link
Contributor Author

ztbrown commented Mar 31, 2021

thank you everyone! 🎉

@renovate renovate bot mentioned this pull request May 21, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yorinasub17 yorinasub17 yorinasub17 approved these changes

@brikis98 brikis98 Awaiting requested review from brikis98

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ztbrown @brikis98 @rhoboat @infraredgirl @yorinasub17