This repository was archived by the owner on Aug 12, 2022. It is now read-only.
fix!: Diagnostic cleanup#418
Closed
shimonp21 wants to merge 3 commits intocloudquery:mainfrom
shimonp21:error_no_errors
Closed
fix!: Diagnostic cleanup#418shimonp21 wants to merge 3 commits intocloudquery:mainfrom shimonp21:error_no_errors
shimonp21 wants to merge 3 commits intocloudquery:mainfrom
shimonp21:error_no_errors
Conversation
The code inside the defaultErrorClassifier was never actually executed, because it was always passed a 'diag', but the first line in defaultErrorClassifier was "if err type is diag, return nil". In any case, it's definitely better not to classify a single error twice. A more reasonable implementation might be to 'fall back to default classifier, if specific classifier returned nil', but for now, let's just remove it.
This commit includes some significant improvements to how error-handling is done in cloudquery. Most importantly - it disallows conversion of Diagnostics (or 'Diagnostic', or 'BaseError') to 'error'. Implicit conversion between diags and errors caused many hard to track bugs (e.g. an empty list of diagnostics becoming an 'error: no errors'). This commit also: - Removes 'IgnoreError' completely. This was kind of dead code - errors marked as ignored could be overwritten and become warnings if the ErrorClassidfier classified them as warnings. This happened through a very convoluted flow converting between diags and errors several times, causing messages such as "warning: cloudquery ignored an error: access denied", instead of a much more straightforward "warning: access denied". - When upgrading a provider to this SDK version, make sure to move any 'ignores' that you require to the ErrorClassifier (i.e. classify them as 'IGNORED' severity). - Changes 'ErrorClassifier' to just receive a single 'error'. It moves all the implicit information that uesd to be passed by creating a diag with context (e.g. resourceID) and passing it to the classifier, to just being explicit parameters. Some more cleanup may be a good idea here. So, the task of an errorClassifier is more straigtforward now - receive an error, convert it to a diagnostic. See the correspoing cq-provider-aws PR for an example of how upgrade the SDK to this version.
shimonp21
requested review from
a team,
roneli,
yevgenypats and
zagronitay
and removed request for
a team
disq
reviewed
Jul 11, 2022
| // Deprecated! Only here for backward compatibility (specifically in the gRPC protocol). Do not use unless | ||
| // you are 100% sure you need this. | ||
| // Converts a list of diagnostics into an error. | ||
| func (diags Diagnostics) DeprecatedToError() string { |
Member
There was a problem hiding this comment.
Suggested change
| func (diags Diagnostics) DeprecatedToError() string { | |
| func (diags Diagnostics) ToError() string { |
Would just mark it as Deprecated: as described in here (The Deprecated! usage above is also non standard)
Contributor
|
I'll wait a bit with this PR to give a shot at removing Diags completely and only using logging mechanism. |
Contributor
|
Closing in favour of #437 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[This PR is split into multiple commits, please review them separately]
This commit includes some significant improvements to how error-handling is done in cloudquery.
Most importantly - it disallows conversion of Diagnostics (or 'Diagnostic', or 'BaseError') to 'error'.
Implicit conversion between diags and errors caused many hard to track bugs (e.g. an empty list of diagnostics becoming
an 'error: no errors').
This commit also:
Removes 'IgnoreError' completely. This was kind of dead code - errors marked as ignored could be overwritten and become warnings if the ErrorClassidfier classified them as warnings. This happened through a very convoluted flow converting between diags and errors several times, causing messages such as "warning: cloudquery ignored an error: access denied", instead of a much more straightforward "warning: access denied".
Changes 'ErrorClassifier' to just receive a single 'error'. It moves all the implicit information that uesd to be passed by creating a diag with context (e.g. resourceID) and passing it to the classifier, to just being explicit parameters. Some more cleanup may be a good idea here. So, the task of an errorClassifier is more straigtforward now - receive an error, convert it to a diagnostic.
See the corresponding cq-provider-aws PR for an example of how upgrade the SDK to this version.
Use the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)