Draft: Validation warnings #272
Conversation
| return tables, nil | ||
| } | ||
|
|
||
| func (c *SourceClient) Validate(ctx context.Context, spec specs.Source) (warnings, errors []string, err error) { |
There was a problem hiding this comment.
Apart from the errors shadow/nit, could maybe consider returning a ValidateResult struct instead of the triple return values? But then it would be very similar to the grpc structs, ValidateDestination_Response etc. Maybe there's a better way? Pass a logger (as interface) and let it log? It's a bunch of lines, wouldn't need any formatting on the CLI side anyway.
There was a problem hiding this comment.
Yeah I'd be good with a struct; it's more extensible in the long run too. I don't think it's an issue that it's similar to ValidateDestination_Response.
| if ok && st.Code() == codes.Unimplemented { | ||
| // Backwards-compatibility with older plugin versions that don't support Validate(). | ||
| // In this case, we only return one warning: that the plugin should be updated. | ||
| return []string{"the version of this plugin is outdated and should be updated"}, nil, nil |
There was a problem hiding this comment.
Nit: Maybe temper the wording a bit.
Some advanced features might not be supported by the current version of the CLI, and might lead to breaks in future. Please update using (this link)
There was a problem hiding this comment.
Thanks, good suggestion - I was struggling to find the right wording here :)
See cloudquery/cloudquery#2612 for an explanation. Notes: - We weren't using `DisallowUnknownFields` on the destination plugin side - We don't currently have a mechanism to warn about unknown fields, but we are working on it #272 - This will unfortunately not fix plugins that have already been released. Our next CLI release that adds new fields will have to be breaking, but after that they can be backwards-compatible.
This adds a backwards-compatible
Validationfunction to the SDK. It's backwards-compatible in the sense that even if it is called from a new CLI using old plugin versions that don't support it, it still works. In this case, a warning is returned that the plugin should be updated.Combined with a few straight-forward changes in the CLI, we can provide users with warnings (such as deprecation warnings) and short, descriptive errors that are easier to read.
Some examples of using this in the CLI:
Opening this as a Draft for some early feedback. If you are happy with the idea, I can tie up some loose ends and add tests.