docs(source/snowflake): remove unsupported timeout field from reference#3626
Open
anxkhn wants to merge 1 commit into
Open
docs(source/snowflake): remove unsupported timeout field from reference#3626anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
The Snowflake source Reference table documented a `timeout` field (integer, "Defaults to 60"), but the Snowflake Config struct has no such field and the source applies no connection timeout anywhere. Source configs are decoded with a strict YAML decoder, so setting `timeout:` in a Snowflake source config is a hard "unknown field" error rather than a no-op, meaning the documented option actively breaks config loading. Remove the fabricated row from the reference table and add a parse-failure test case asserting that `timeout: 60` is rejected, so the docs and the strict-decode behavior stay in sync. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request removes the unsupported timeout configuration parameter from the Snowflake source documentation and adds a unit test to verify that configuring a timeout field results in an unmarshaling error. I have no feedback to provide as there are no review comments.
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 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.
The Snowflake source Reference table documents a
timeoutfield(
integer, "The connection timeout in seconds. Defaults to 60."), but theSnowflake source has no such option:
Configstruct ininternal/sources/snowflake/snowflake.godeclares only
name,type,account,user,password,database,schema,warehouse, androle. There is notimeoutfield, andinitSnowflakeConnectionnever applies a connection timeout.yaml.Strict()ininternal/util/util.go), so an unknown key is a hard error, not a silentlyignored no-op.
The net effect is that a user who follows the docs and adds
timeout: 60to aSnowflake source config does not get a 60-second timeout: their configuration
fails to load with an
unknown field "timeout"error. The documented optionactively breaks config loading.
This PR removes the fabricated
timeoutrow from the reference table. Thewarehouseandrolerows are left untouched: those defaults ("COMPUTE_WH" /"ACCOUNTADMIN") really are applied in
initSnowflakeConnection, so the docs arecorrect there.
It also adds a table-driven parse-failure case to
TestFailParseFromYamlininternal/sources/snowflake/snowflake_test.goasserting that a config withtimeout: 60is rejected. This ties the doc change to observable behavior andguards against the field being re-documented without being implemented.
Scope note: adding an actual connection timeout would be a
feat; this PR isthe surgical
docsfix that makes the reference match current behavior.PR Checklist
!if this involves a breaking changeIssue Reference