Should we define the tap default settings in the plugin definition yaml?

[pnadolny13]

This has come up in many places, most recently in #1080 so I wanted to start a discussion that we can keep as a reference for our decision if it comes up again.

Theres a value attribute that you can assign to a setting that acts as a default value. Theres also a default attribute in the tap config definition (e.g. target-postgres port).

The way that the plugin definition attribute works right now is that it populates the config.json with the default value if the setting is not configured by the user in their project. This could potentially be troublesome if we set the value based on the current default but the tap changes their implementation and the default in the tap is different from the meltano metadata, we're now influencing the tap to run in a non-default way.

Theres two reasons why we'd want to set the value attribute:

1. To include metadata for UX reasons, e.g. things like UI or interactive config can show the default value. In legacy singer tap defaults are sometimes scattered around the repo so its helpful for us as metadata maintainers to expose them clearly.
2. For us to override the default behavior of the tap or providing extra defaults to simplify the onboarding experience. I think we've also done this in the past to avoid tap defaults that we dont think are ideal for new users.

History:

• At first we were defining value whenever we saw a default value in the tap
• Then we moved away from that to avoid mismatched defaults.
• Now we dont have good metadata in interactive config

@tayloramurphy @aaronsteers @visch How do you think we should handle this moving forward?

[pnadolny13]

I had two initial ideas:

1. Only set defaults for SDK based variants once we have automation to regularly check for updates, so we're confident that we're not out of sync.
2. Add an additional attribute to the plugin definitions schema like default_override. This way we'll have a way to define the default as metadata using value and default_override to..override, if needed.

[aaronsteers]

We can't change the behavior of value without breaking existing implementations. When we want to document 'default behavior', I think we should just put that into description. This is a pretty standard approach and doesn't require any new fields to be created. Then, if the plugin changes its defaults, that is a documentation bug and requires an update.

If we want to freeze the default or if we want to set a Meltano specific default like "output": "$MELTANO_PROJECT_ROOT/output", then we should set that frozen/overridden default into value.

Long-term, if we want to display the plugin's own default values, and if we want these to stay up-to-date, we'll need to automate the --about call to the plugin and then pass that to the interactive prompt, and that's a fairly expensive process in terms of dev hours, and would only help for SDK-based connectors.

[pnadolny13]

When we want to document 'default behavior', I think we should just put that into description.

@aaronsteers I agree, I think this is what we landed on last time we discussed this. But yeah the downside is that we can't show it in things like interactive config like Derek was calling out.

Long-term, if we want to display the plugin's own default values, and if we want these to stay up-to-date, we'll need to automate the --about call to the plugin and then pass that to the interactive prompt...

I think this is similar to my idea 1 from above but having --about called during config is even better. If the metadata is retrieved in an automated way (i.e. daily/weekly hub refresh job or at runtime for interactive config) then I'm not worried about the defaults being out of sync anymore. Those options are only available for SDK based plugins though.

[visch]

My main thought is that saying the default is "Empty" even though it's not empty is probably wrong unless we tell people that the tap may have defaults of its own that are not reflected here. I think that could be put in the section under the description.

Like "Note that empty string means that we are not passing anything in config to the tap. The tap may have its own defaults set that are not reflect here. To find them please look at the tap repository"

---

What is actually wrong is the prompt says that we will be passing a port of "empty string" even though we won't be (This is kind of confusing to talk about but I think if we get it right it becomes intuitive). The setting port won't be listed in config.json at all which is different than setting port to null or an empty string (This isn't just an interactive issue). If we had a way of saying whether the config key would be sent in config.json then life gets a bit easier as you could say default is Not being set, depends on tap implementation or Not sent, delegated to the tap in this case because we're not sending a port at all. This also would solve the stream_maps issue we brought up a bit ago that I think edgar fixed with the "_NULL" workaround value or something?

@pnadolny13 this paragraph sounds really good in my head right now for a long term fix but I might be over thinking it.

[aaronsteers]

Agreed "empty" is not the right description.

I think when not set, the current/default value should be something like "current value: none", "default value: none".

And the description should just say someone like:

If not specified, [some behavior].