-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Replace customerrors with go-multierror #764
Conversation
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 for the contribution, the code looks good!
I left a couple of suggestions inline:
- Using
new
is more readable when creating a new multierror - We should always return
.ErrorOrNil()
as that takes care to returnnil
in the case there were no errors
Co-authored-by: Ana Krivokapić <[email protected]>
Co-authored-by: Ana Krivokapić <[email protected]>
Co-authored-by: Ana Krivokapić <[email protected]>
Co-authored-by: Ana Krivokapić <[email protected]>
Co-authored-by: Ana Krivokapić <[email protected]>
Co-authored-by: Ana Krivokapić <[email protected]>
Thanks for the suggestions @infraredgirl, I will keep them in mind. |
Thanks for the updates, Rohit! The code LGTM now, but tests seem to be failing. This could be due to some unrelated test instability we have hit lately. There's a PR open that should stabilize tests a bit, I'll wait for that to merge before merging this. Thanks for your patience! |
Thanks for the update, Ana. Sorry I saw your msg late. |
FYI - #765 is now merged. |
Thanks Rob! I re-ran the tests and the only failure is in helm tests which were not affected by this change. I'll go ahead and merge - thanks again for the contribution @urohit011 ! |
Thank you Ana and Rob :) |
This pull request resolves #751 by replacing the use of customerrors with go-multierror
Link to test result gist -> gist
The test github.com/gruntwork-io/terratest/test/terraform_scp_example_test.go fails, but it does not look like it's due to the change in this commit.