fix: Don't call RowsAffected on query error#17568
fix: Don't call RowsAffected on query error#17568kodiakhq[bot] merged 6 commits intocloudquery:mainfrom
RowsAffected on query error#17568Conversation
erezrokah
requested review from
a team and
bbernays
and removed request for
a team
| if rowsErr != nil { | ||
| return rowsErr | ||
| } | ||
| c.logger.Debug().Str("query", query).Any("values", args).Int64("rowsAffected", rowsAffected).Msg("exec query") |
There was a problem hiding this comment.
This wouldn't log rowsErr or the query err and you'd only get debug messages for successful queries.
There was a problem hiding this comment.
Cool, so I guess we can keep this log 👍
There was a problem hiding this comment.
Well actually won't returning the error make it so it's logged always?
There was a problem hiding this comment.
Anyway maybe keep it since we don't want to fail if we couldn't write a debug log
There was a problem hiding this comment.
Actually when I look at the previous code, we would return rowsErr even if err was nil. Not sure that is what we want if the intention was only to log it
There was a problem hiding this comment.
Previous code would return all errors (if any) thanks to the errs := errors.Join(err, rowsErr) line.
There was a problem hiding this comment.
Previous code would return all errors (if any) thanks to the
errs := errors.Join(err, rowsErr)line
It won't since it panics in case of initial error 🙃
There was a problem hiding this comment.
c11c402 isn't the same as before, ideally if c.spec.Debug is on, it should log the query and error before returning (not running rows affected)
An easier approach could be to check if err is nil inside c.spec.Debug and run rowsaffected and join the errors (or just don't care about the rowsaffected error) inside the block.
erezrokah
force-pushed
the
fix/dont_call_rows_affected
branch
from
249a668 to
011c48b
Compare
🤖 I have created a release *beep* *boop* --- ## [1.0.6](plugins-destination-motherduck-v1.0.5...plugins-destination-motherduck-v1.0.6) (2024-04-09) ### Bug Fixes * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.36.4 ([#17485](#17485)) ([f370de4](f370de4)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.36.5 ([#17526](#17526)) ([554c499](554c499)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.37.0 ([#17532](#17532)) ([8080970](8080970)) * Don't call `RowsAffected` on query error ([#17568](#17568)) ([68cf566](68cf566)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- ## [5.5.3](plugins-destination-duckdb-v5.5.2...plugins-destination-duckdb-v5.5.3) (2024-04-09) ### Bug Fixes * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.36.4 ([#17485](#17485)) ([f370de4](f370de4)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.36.5 ([#17526](#17526)) ([554c499](554c499)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.37.0 ([#17532](#17532)) ([8080970](8080970)) * Don't call `RowsAffected` on query error ([#17568](#17568)) ([68cf566](68cf566)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
3 participants
Summary
Fixes the panic from https://github.com/cloudquery/cloudquery/actions/runs/8607255034/job/23587408786?pr=17562#step:8:10