Skip to content

Conversation

@sjrl
Copy link
Contributor

@sjrl sjrl commented Oct 21, 2025

Related Issues

Proposed Changes:

  • Reverts the breaking change of feat: add MFA authentication support to snowflake integration #2305 by setting a default value for authenticator which matches the previous default behavior. We also set defaults for all Secrets instead of setting them to None by default.
  • Move auth validation and database connection testing to the warm_up method of SnowflakeTableRetriever. This allows for the component to be initialized without needing all secrets present. This is useful if you want to perform Pipeline validation without needing all secrets present.
    • This is a breaking change because it requires calling warm_up on the component before it can be used. This is only relevant if you are using this component in isolation. If being used in a Pipeline then the Pipeline will automatically call warm_up on it so usage of this component does not change.

How did you test it?

Notes for the reviewer

Checklist

@sjrl sjrl requested a review from a team as a code owner October 21, 2025 07:39
@sjrl sjrl requested review from anakin87 and medsriha and removed request for a team October 21, 2025 07:39
@github-actions github-actions bot added integration:snowflake type:documentation Improvements or additions to documentation labels Oct 21, 2025
@sjrl sjrl self-assigned this Oct 21, 2025
@sjrl
Copy link
Contributor Author

sjrl commented Oct 21, 2025

@medsriha would it be possible for you to live test this component against a live Snowflake db? I noticed that we don't have any integration tests for this core integration so it would be good to confirm that it does work as expected.

Copy link
Member

@medsriha medsriha left a comment

Choose a reason for hiding this comment

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

Tested on real credentials. Looks good!

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

I am OK with this change, also given the low usage of this integration.

Let's remember to:

@sjrl sjrl merged commit 9555069 into main Oct 22, 2025
10 checks passed
@sjrl sjrl deleted the add-warm-up-to-snowflake branch October 22, 2025 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration:snowflake type:documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants