Bug: meltano config meltano set should always set a value

[DouweM]

~/Development/meltano-projects/superset-test ❯❯❯ meltano config meltano set discovery_url http://localhost:4000/discovery.yml
2022-06-04T01:36:33.566497Z [info     ] Environment 'dev' is active
Meltano setting 'discovery_url' was set in `meltano_environment`: 'http://localhost:4000/discovery.yml'
Current value is still: 'https://discovery.meltano.com/discovery.yml' (from the default)

I'd expect this to store discovery_url: http://localhost:4000/discovery.yml in meltano.yml, but it's nowhere to be found.

[DouweM]

This also seemed odd, but may be complicated by the fact that I have tracking disabled through an env var. Still shouldn't break setting it in meltano.yml, though!

~/Development/meltano-projects/superset-test ❯❯❯ meltano config meltano set send_anonymous_usage_stats true        (meltano-dev) 
2022-06-04T01:28:15.202752Z [info     ] Environment 'dev' is active
Meltano setting 'send_anonymous_usage_stats' was set in the default: True
Current value is still: False (from the environment)

This ended up removing send_anonymous_usage_stats: false from meltano.yml, which makes sense

~/Development/meltano-projects/superset-test ❯❯❯ meltano config meltano set send_anonymous_usage_stats false       (meltano-dev) 
2022-06-04T01:28:39.954507Z [info     ] Environment 'dev' is active
Meltano setting 'send_anonymous_usage_stats' was set in `.env`: False
Current value is still: False (from the environment) 

This didn’t add send_anonymous_usage_stats: false to meltano.yml as I’d expect, but .env instead

[DouweM]

@aaronsteers I've set the urgency to "Higher" here because this could be a bad bug.

[aaronsteers]

@DouweM - Makes sense. I'm not sure when this started but it's on my radar too. Thanks.

[aaronsteers]

My guess is we have two bugs here:

• first is in resolution order and/or finding the location store
• second is that Meltano is trying to be too clever and clear values instead of writing new ones, if it thinks the new value matches something set at same/higher precedence.

The second issue, I think, is the bigger issue we should tackle. The first issue might only be affecting this particular code path and (if so) might be best resolved by not trying to infer a hierarchy at all.

As a user, if I tell Meltano to store a setting, I don't want Meltano to second guess whether it actually needs to be set.

I'll take a deeper look tonight and @kgpayne I might assign to you since this is a part of the codebase I know you are very familiar with.

[DouweM]

@aaronsteers Note that the title here now only reflects the issue in #6015 (comment), not #6015 (comment), which I think its the more urgent issue and possibly new bug.

[aaronsteers]

@DouweM - The more I've looked into this, the more I think these appear to be the same issue.

"Nowhere to be found" seems to be a symptom of the write operation being short-circuited before writing - and 'removing conditional write with bad checks' would by definition solve the 'nowhere to be found' issue.

Regardless of the circumstances in which we call config set, meltano should always go through four steps:

1. pick the most appropriate store if not explicitly set,
2. write the value to the selected store,
3. correctly print to the user that the value was saved and where the value was stored,
4. correctly print some help text about whether or not the value set is expected to be used based on other stores which may override it.

Some of your sample cases (from your logs, at least) are definitely failing the 2nd and 3rd steps. They may also be failing the fourth step if they are printing bad information about what the effective value is after the set operation - but that I'm less sure of as of now.

If there's more to address, can you clarify?

[DouweM]

@aaronsteers I haven't tested #6025 yet to see if it actually resolved my first issue, but I don't understand how it would, as the code being removed there compares the new value against the default or current value, but in my case the new value (http://localhost:4000/discovery.yml) is neither, so these code branches should already not be entered. Also, those code branches return store: DEFAULT or store: <current store> respectively, while my log output shows that store was meltano_environment. That suggests to me that the meltano_environment-specific code path was entered and failed to update to meltano.yml, possibly around here: https://github.com/meltano/meltano/blob/main/src/meltano/core/settings_store.py#L763

Were you able to reproduce my first issue and verify that this changed solved it? If so, disregard anything I said here, although I'd still be concerned about Chesterton's fence (#6025 (comment)).

If you have trouble reproducing the issue let me know, and I can dig into it some more.

[aaronsteers]

@DouweM - We're getting a little deep into philosophy here 🧠 😅 (TIL) but I appreciate the points.

What I'm not sure of is if we're on the same page regarding the symptom we want to solve. I have a few other PRs that need my attention but I'll come back to this when I can. While I appreciate the Chesterson's fence argument, we can also just decide that the simpler and more obvious behavior is the better one: said simply:

• set a value when a set is requested
• put it in the right place
• report back how it went

All of the symptoms you are describing are (I think) encompassed in those requirements, and in the 4 bullets I posted above.

[DouweM]

@aaronsteers All I want is for meltano config meltano set discovery_url http://localhost:4000/discovery.yml to work. I can reproduce it like this on the main branch:

$ meltano init 6015-test
# [snip output]
$ cd 6015-test
$ cat meltano.yml
version: 1
default_environment: dev
send_anonymous_usage_stats: false
environments:
- name: dev
- name: staging
- name: prod
$ meltano config meltano set discovery_url http://localhost:4000/discovery.yml
2022-06-05T18:25:24.365695Z [info     ] Environment 'dev' is active
Meltano setting 'discovery_url' was set in `meltano_environment`: 'http://localhost:4000/discovery.yml'
Current value is still: 'https://discovery.meltano.com/discovery.yml' (from the default)
$ cat meltano.yml
version: 1
default_environment: dev
send_anonymous_usage_stats: false
environments:
- name: dev
- name: staging
- name: prod

If #6025 indeed fixes that, let's discuss the merits of that behavior change more, but otherwise I don't think we need to try to get that resolved before 2.0.

[DouweM]

@aaronsteers Let me know if you'd like me to look into this myself! I think I still know this part of the codebase pretty well.

[DouweM]

Fixed in #6034!