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

Allow for ssm invoke without client creation #924

Merged
merged 1 commit into from
Jun 25, 2021
Merged

Allow for ssm invoke without client creation #924

merged 1 commit into from
Jun 25, 2021

Conversation

danquack
Copy link

@danquack danquack commented Jun 10, 2021 •
edited
Loading

In line with the #797, I had a use case for not creating a client with the other SSM error functions. This PR should keep in line with current functionality, but extend additional WithClient functions to allow for some flexibility.

brikis98
brikis98 reviewed Jun 17, 2021
View reviewed changes
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Code changes LGTM, except one minor thing: could you please add comments to the new functions, following the comment format of the existing functions?

brikis98
brikis98 previously approved these changes Jun 18, 2021
View reviewed changes
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! I'll kick off tests now.

@danquack
Copy link
Author

It appears TestDoInBackgroundUntilStopped failed, but trying to read through some of the code I’m not sure if that relates back to this PR. Can you confirm if any action is required on my part for this @brikis98 ?

@brikis98
Copy link
Member

I'm not sure, TBH! I know we recently merged some fixes for flaky tests to master. Would you mind pulling the latest from master and I can kick off tests again?

@danquack
Copy link
Author

Rebased

@brikis98
Copy link
Member

Thanks! I'll kick off tests again.

@danquack
Copy link
Author

Darn, looks like that broke more than the last time!

@brikis98
Copy link
Member

Yes, sorry about that. I think it might be caused by some new validate tests we added that, when run all concurrently, was overloading the CircleCi servers, and causing sporadic test failures. We fixed that in #938. Apologies for all the back and forth, but would you mind pull in in the latest from master once more?

@danquack
Copy link
Author

No worries. We've all been there. Just thankful for the fast feedback. Rebased and squashed.

@brikis98
Copy link
Member

Thanks! Re-running tests now.

brikis98
brikis98 approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Alright, tests passed! Merging now.

@brikis98 brikis98 merged commit 0122e76 into gruntwork-io:master Jun 25, 2021
@danquack danquack deleted the 797 branch June 25, 2021 16:39
@brikis98
Copy link
Member

https://github.com/gruntwork-io/terratest/releases/tag/v0.36.3

@danquack danquack mentioned this pull request Jun 25, 2021
Closed
This was referenced Jun 25, 2021
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brikis98 brikis98 brikis98 approved these changes

@yorinasub17 yorinasub17 Awaiting requested review from yorinasub17

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

Successfully merging this pull request may close these issues.
2 participants
@danquack @brikis98