-
Notifications
You must be signed in to change notification settings - Fork 92
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
feat: terramate generate: add overall tf code generation #193
Conversation
…-add-tf-code-to-generate
…-add-tf-code-to-generate
Codecov Report
@@ Coverage Diff @@
## main #193 +/- ##
==========================================
+ Coverage 61.90% 62.82% +0.91%
==========================================
Files 30 30
Lines 5064 5197 +133
==========================================
+ Hits 3135 3265 +130
- Misses 1755 1757 +2
- Partials 174 175 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Lgtm but also a bit over my head.
generate/generate_hcl_test.go
Outdated
} | ||
workingDir := filepath.Join(s.RootDir(), tcase.workingDir) | ||
err := generate.Do(s.RootDir(), workingDir) | ||
assert.IsError(t, err, tcase.wantErr) |
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.
are we actually setting wantErr
in any of the test cases? if this is not the case, shouldn't this assertion fail?
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.
If we never set wantErr, meaning that we are not expecting a failure, and no failure happens, this is the same as:
assert.IsError(t, nil, nil)
Which works, because nil == nil (they match). If we get an unexpected error, it fails, and if we define wantErr then it would fail if the got err doesn't match.
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.
But since we are not using it here we can remove it, I usually have some error handling tests but so far haven't added at this level. But here you can see how it works:
When you set the error, it will check for a specific match with the sentinel wrapped inside the error, when you don't set it it will check if both errors are nil, since only a nil error "is" another nil error.
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.
Simpler example: https://go.dev/play/p/aMat5TWAU7t
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.
got it! thanks for taking the time to explain! ❤️
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.
dude, looks great to me! 🧙♂️
I added some questions but nothing should be blocking - just https://github.com/mineiros-io/terramate/pull/193/files#r798043896 but I assume I'm missing something since the tests are green)
Thanks for the review man ❤️ Tried to explain and provide some examples about errors.Is, the assert.IsError is a wrapper around errors.Is, so they behave the same way. Also improved the code with all your suggestions, thanks again ❤️ |
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.
lgtm! 🚀
Introduces generate_hcl code generation on the cli, including outdated code detection. Here is an example to showcase how to use + expected results: