materialize-sql: add support for checking prerequisites during validation #612
Conversation
This file contains 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
…tion Adds the field CheckPrerequisites to the Endpoint struct created by all sql materializations. This field is a function that will be called in Validate.
Adds prerequisite checks during validation & a configuration check to enforce that the snowflake host configuration must end with ".snowflakecomputing.com". This might not work if we ever support things like AWS PrivateLink, but for now it allows an easy way to prevent errors with this configuration field. Also adds a test for Snowflake fencing which is entirely unrelated to these changes, but seemed nice to do while I was updating tests.
mdibaiee
approved these changes
Mar 21, 2023
materialize-snowflake/snowflake.go
Outdated
| @@ -88,12 +96,14 @@ func (c *config) Validate() error { | |||
| return fmt.Errorf("missing '%s'", req[0]) | |||
| } | |||
| } | |||
|
|
|||
| if !validHost(c.Host) { | |||
| return fmt.Errorf("invalid host %q (must match end in snowflakecomputing.com and not include a protocol)", c.Host) | |||
There was a problem hiding this comment.
Nit: I think it's better if validHost itself gives this error, since that function knows what it is checking, I would expect the caller to not have to know what is happening inside the function
oliviamiannone
added
the
docs complete / NA
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description:
Adds a "prerequisites" check to sql materializations that is invoked during
Validatecalls.Currently there are some ad-hoc validations that occur during
NewEndpoint. These are not comprehensive, and not including them inValidatemeans that they don't run duringTestinvocations. The comprehensive validations are more involved and moving them fromNewEndpointtoValidateis also nice since they won't have to run every time the connector starts up, and are only run when a change to the materialization is being considered.I generally tried to interpret errors for common user-error cases and return clear & helpful error messages for these cases. This does not always work as well as I'd like depending on what the client library gives us, but overall I think we will have things covered pretty well here.
There is the potential for multiple errors to be independently evaluated for some materializations. In this case we will report as many errors as possible rather than stopping on the first one. This is really only the case for redshift and bigquery which use staging buckets and have the potential for separate client configuration and cloud storage configuration errors.
Also note that there is a small tweak to the snowflake configuration validation: We will now only accept host URLs ending in
.snowflakecomputing.com. It has never been possible for the materialization to work with anything other than*.snowflakecomputing.com, but now the connector will validate that specifically when parsing the configuration. This helps eliminate a lot of possible configuration errors that would have to otherwise be validated in the prereq check for the connector.Workflow steps:
Use connectors like before, but have better error messages when trying to publish them.
Documentation links affected:
I'll take a look at the snowflake docs and see if there is anything that needs updated there.
Notes for reviewers:
(anything that might help someone review this PR)
This change is