Skip to content
This repository has been archived by the owner on Aug 12, 2022. It is now read-only.

feat!: Validate provider config #435

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

disq
Copy link
Member

@disq disq commented Jul 22, 2022

Implements cloudquery/cloudquery#1104
I'm not sure if this is really necessary at this point, as we decode the config before validating schema anyway. If it doesn't decode cleanly we just stop there and don't go into the schema validation.

This PR also starts decoding the config in strict mode, unknown keys in the config will produce errors from now on.

@disq disq requested a review from a team as a code owner July 22, 2022 21:24
@disq disq requested review from amanenk and removed request for a team July 22, 2022 21:24
@disq disq requested a review from roneli July 22, 2022 21:27
@disq disq changed the title feat: Validate provider config feat!: Validate provider config Jul 22, 2022
var diags diag.Diagnostics

// validate provider configuration before proceeding
validateResponse, err := p.ValidateProviderConfig(ctx, &cqproto.ValidateProviderConfigRequest{Config: request.Config})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every call to ConfigureProvider also runs ValidateProviderConfig. This also means we decode and defaultize the config twice: once in validate and one down below.

@yevgenypats
Copy link
Member

yevgenypats commented Jul 23, 2022

Maybe worth doing a live walkthrough on this to go through all the cases. One questions in the meanwhile:

  1. why not just send the schema json as string to core from the the provider and make the core do the validation? this will make remove the need to send all the diags stuff from provider to core and we can use normal errors?

@yevgenypats yevgenypats requested review from yevgenypats and removed request for amanenk July 23, 2022 08:36
@yevgenypats
Copy link
Member

Im working on a complete re-write of cloudquery core including no-diags so let's put this on hold for now as otherwise I wont be able to merge:
cloudquery/cloudquery#1139
#437

Copy link
Member

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going into a feature freeze in cloudquery core and sdk apart from P0 bugs until cloudquery v2 is done https://github.com/cloudquery/cloudquery

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants