fix: Recover panic in table resolver and object resolver flows#257
fix: Recover panic in table resolver and object resolver flows#257kodiakhq[bot] merged 7 commits intomainfrom
Conversation
This also adds quite needed tests for our scheduler
| if tc.Summary.Resources != i { | ||
| t.Errorf("expected %d resources, got %d", tc.Summary.Resources, i) | ||
| } | ||
| if tc.Summary.Resources != summary.Resources { | ||
| t.Errorf("expected %d summary resources, got %d", tc.Summary.Resources, summary.Resources) | ||
| } |
There was a problem hiding this comment.
nit: Looks like the second comparison here can never fail except if either: the case above it already failed, or the test is set up incorrectly. I'd probably remove the != i check
There was a problem hiding this comment.
it can (It did and this is why I've added it :) ) because the first check checks how many resources were returned on the channel and the second one just checks the statistics struct
There was a problem hiding this comment.
Oh I see now, sorry yeah. It's because it's written in want != got instead of got != want order that I misread it. My nit would then be to change the ordering :)
There was a problem hiding this comment.
I think it is usually expected != got but also sometimes it's hard to keep the order consistent because for example you can't do 1 != some_var so you still have constraints.
There was a problem hiding this comment.
In Go it's usually got != expected, so that it's consistent when you have a constant got != 1 as well. https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures
hermanschaaf
left a comment
There was a problem hiding this comment.
Nice, I like the tests. LGTM 👍
| sentry.WithScope(func(scope *sentry.Scope) { | ||
| scope.SetTag("table", t.Name) | ||
| sentry.CurrentHub().Recover(err) | ||
| sentry.CurrentHub().CaptureMessage(stack) |
There was a problem hiding this comment.
Sentry should already include the stack trace?
There was a problem hiding this comment.
their logic is incorrect it doesn't print it in the right way and try to do some "smart things" - just a string of the stack trace should work for us I think.
|
@disq @hermanschaaf can you please take a look at the lint error - https://github.com/cloudquery/plugin-sdk/actions/runs/3203350691/jobs/5233341574#step:4:26 I think it's a false positive but just want to be sure before I ignore it. |
@yevgenypats I don't see how it can be a datarace, as we close |
|
That's right. I think it is a false positive because the linter doesn't detect that Im using |
|
|
@yevgenypats I'm not 100% sure it is a data race either, but rather than ignore the warning, let's pass in a pointer to the goroutine so that it's definitely safe? go func(syncSummary *SyncSummary) {
...
atomic.AddUint64(&syncSummary.Panics, 1)
...
atomic.AddUint64(&syncSummary.Errors, 1)
...
}(&summary) |
|
good idea. done. |
| meta.Logger().Trace().Str("table", t.Name).Msg("post resource resolver finished successfully") | ||
| } | ||
| } | ||
| summary.Resources = 1 |
There was a problem hiding this comment.
| summary.Resources = 1 | |
| summary.Resources++ |
🤖 I have created a release *beep* *boop* --- ## [0.12.8](v0.12.7...v0.12.8) (2022-10-07) ### Bug Fixes * Error on incorrect table configuration ([#237](#237)) ([6ad75f5](6ad75f5)) * Exit gracefully on context cancelled ([#252](#252)) ([b4df92e](b4df92e)) * Progressbar should go into stdout ([#250](#250)) ([b8bcdad](b8bcdad)) * Recover panic in table resolver and object resolver flows ([#257](#257)) ([04dba02](04dba02)) * Stop if PreResourceResolver fails ([#251](#251)) ([ee83f8f](ee83f8f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This also adds quite needed tests for our scheduler