Fix "error: no errors"

[shimonp21]

Describe the bug

An interesting bug that stems from the fact that diag.Diagnostics implements the error interface (i.e. func Error() string {:
both diag.BaseError and diag.Diagnostics (i.e. a slice of diagnostics []diag.Diagnostics ) implement the error interface:

type error interface {
	Error() string
}

This means that, if a function returns diag.Diagnostics , but thinks it returns an error, it will be implicitly converted. And thus, even though the function returned nil, the caller will actually see error: no errors.
To reproduce:

func Test_DiagAndErrors(t *testing.T) {
	var err error

	err = functionReturningDiags0()
	if err != nil {
		fmt.Printf("error: %vֿ\n", err)
	}

	err = functionReturningDiagsNil()
	if err != nil {
		fmt.Printf("error: %vֿ\n", err)
	}
}

func functionReturningDiagsNil() diag.Diagnostics {
	return nil
}

func functionReturningDiags0() diag.Diagnostics {
	return diag.Diagnostics{}
}

Output:

error: no errorsֿ
error: no errorsֿ

Insights:
A customer complained about error: no errors in discord a couple of weeks ago (in relation to policies).
implicit conversion is bad! The code for converting Diagnostics to errors makes me believe that this is a side effect that the writer did not intend.

cq-provider-sdk/provider/diag/diagnostics.go

Line 18 in 22a7afb

func (diags Diagnostics) Error() string {

My proposal would be to rename Diagnostics.Error() to Diagnostics.ToError() to avoid implementing the error interface and allowing this implicit conversion.

Expected Behavior

Steps to Reproduce

Possible Solution

No response

Provider and CloudQuery version

0.24.7

Additional Context

No response

[shimonp21]

Summary of meeting today:

• diags were copied from terraform code https://github.com/hashicorp/terraform-plugin-sdk/blob/112e2164c381d80e8ada3170dac9a8a5db01079a/internal/tfdiags/diagnostics.go#L109.

• Making the conversion to error explicit is a good idea, but it may require writing a wrapper "DiagnosticsAsError", similar to hashicorp:

  • Resolvers in providers return error. In cases where we want to return a warning, we won't be able to return diagnostics, but will have to return the "DiagnosticsAsError" wrapper (and unwrap it in the SDK).

• prioritisation of this is TBD.

[shimonp21]

The issue from cloudquery/cloudquery#772 was already fixed.

[shimonp21]

Also, the single "Diag" might also not want to support the error interface.

looking at the code of our aws and GCP providers, I see only 1 place where we actually return a diagnostic and not an error from a resolver.

[shimonp21]

Sub issue (not huge priority):

column resolvers don't always mention which resource failed.

Resource: ec2.vpcs   Type: Access Severity: Warning
        Summary: column resolver "tags" failed for table "aws_ec2_vpcs": shimon is not authorized to perform something
        Resource ID: 615713231484,eu-west-3
        Detail: Something is wrong with your credentials. Ensure you have access to the specified resource.

I believe that this is because we don't resolve the PKs first...

[shimonp21]

Moving to icebox per @yevgenypats request