Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Support SINGER_SDK_* prefix for built-in settings like flattening_enabled, etc. #1379

Closed
edgarrmondragon opened this issue Feb 1, 2023 · 2 comments
Labels
kind/Feature New feature or request valuestream/SDK

Comments

@edgarrmondragon
Copy link
Collaborator

edgarrmondragon commented Feb 1, 2023

Feature scope

Configuration (settings parsing, validation, etc.)

Description

Currently all settings in a plugin's config schema can be passed via env vars with the plugin name as prefix (e.g. TAP_STACKEXCHANGE_KEY), this includes SDK-provided settings like flattening_max_depth.

It would be useful to support a global env var that configures all SDK plugins to have the same behavior, so SINGER_SDK_FLATTENING_MAX_DEPTH=1 would limit flattening of all SDK taps in a project.

This would also be useful in CI, where developers may want to limit the sample size (#1333) with SINGER_SDK_MAX_RECORDS without having to configure it for each tap.

@aaronsteers
Copy link
Contributor

aaronsteers commented Feb 8, 2023

I think I prefer to not have a generic SINGER_SDK_ prefix, primarily due to the risk of accidental leakage across taps. Also, accepting SINGER_SDK_ as a prefix would introduce the need to handle resolution logic and/or exception handling if SINGER_SDK_SOMESETTING and TAP_FOOBAR_SOMESETTING are both set.

I think a best practice of configuring taps directly.

Also, per the docs, I think the TAP_FOOBAR_ prefix is only parsed if --config=ENV is provided in the command line - in favor of preferring explicit config values to come from config.json while still allowing users to opt in to the convenience benefit.

One reason to not auto-capture env vars would be to avoid double-parsing when run through something like Meltano, which already passes those same/similar config values via similarly named env vars. Note that the natural plugin name might not match Meltano's alias - and in those cases, we wouldn't want settings to impact both/all inherited plugins without the user realizing that TAP_POSTGRES_HOST would affect tap-postgres and also tap-postgres-myalias.

@edgarrmondragon
Copy link
Collaborator Author

@aaronsteers Yeah, thanks for the feedback. Since this is a pattern already support by Meltano as MELTANO_EXTRACTOR__<SETTINGNAME>, I'll close this.

@edgarrmondragon edgarrmondragon closed this as not planned Won't fix, can't repro, duplicate, stale Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/Feature New feature or request valuestream/SDK
Projects
None yet
Development

No branches or pull requests

2 participants