Add tests and docs for environment variable resolution.

[MeltyBot]

Migrated from GitLab: https://gitlab.com/meltano/meltano/-/merge_requests/2667

Originally created by @cjohnhanson on 2022-05-26 21:59:55

---

Merge Request Checklist

• Link to an issue
• Include the proposed fix or feature
• Include and update tests for the modified code
• Include a documentation change
• Add a CHANGELOG.md entry in the Unreleased section for any user-facing changes
• Mention or assign a maintainer

Closes #3173

[tayloramurphy]

@aaronsteers @cjohnhanson I'm realizing that the configuration page https://docs.meltano.com/guide/configuration doesn't mention the env: key at all...

@cjohnhanson I think for this PR we need something in the Environment variables section that mentions that key.

@aaronsteers so as not to completely blow up scope on the PR, I can open an additional issue around the env key since we currently have that info scattered all around.

[tayloramurphy]

@aaronsteers I opened #5969

[aaronsteers]

@cjohnhanson - Do you mind adding summary of tests/changes here, and especially if there are any additional tasks or issues to resolve?

My expectation was that some of our proposed tests would fail and perhaps need to be opened in new issues if scope was too large to fit in the scope of this PR.

Are there any cases we are not yet succeeding at, and/or are there any other caveats that should be called out before merging this PR and closing the related issue?

cc @tayloramurphy

Thanks!!

[cjohnhanson]

@aaronsteers -

Do you mind adding summary of tests/changes here, and especially if there are any additional tasks or issues to resolve?

Sure thing, the high-level takeaway is that there are 7 tests which currently xfail (rather than hard fail, which is why you won't see these failures in the pipeline, they'll show as XFAILs in pytest output). These 7 tests can be resolved by addressing two underlying causes.

Shell environment variables take too high of precedence compared to what is described in the miro board, I'll create a follow-up issue for that which will resolve 6 of the currently `xfail'-ing tests.

Inheritance (e.g., setting a STACKED env var to 1 and then {STACKED}2, etc.) also basically doesn't work at all as expected. I'll create another follow-up issue to address that which will resolve the one remaining failing test case.

To be clear though, the behavior is in line with what our docs currently say, but not with the new model as described in the miro board.

and/or are there any other caveats that should be called out before merging this PR and closing the related issue?
I still need to address this feedback from @tayloramurphy :
@cjohnhanson I think for this PR we need something in the Environment variables section that mentions that key.

See also this comment in the gitlab MR for some more details:

The tests I've written are in line with my reading of the "New: Simplified Model" for precedence in the miro board, but differ significantly from current behavior. The tests corresponding to the scenarios in which current behavior is out of line with desired behavior are currently set to xfail and are parametrized with human-readable scenario descriptions to facilitate this follow-up work. The current behavior for precedence matches what's currently in the docs, though I added a short clarification on environment-level precedence for plugin settings.The tests I've written are in line with my reading of the "New: Simplified Model" for precedence in the miro board, but differ significantly from current behavior. The tests corresponding to the scenarios in which current behavior is out of line with desired behavior are currently set to xfail and are parametrized with human-readable scenario descriptions to facilitate this follow-up work. The current behavior for precedence matches what's currently in the docs, though I added a short clarification on environment-level precedence for plugin settings.

[cjohnhanson]

@aaronsteers @tayloramurphy I'm hoping to get to a PR tonight for my one other remaining 2.0 issue. So if I misunderstood scope here and we need to try to address one or both of the resolution/inheritance issues I outlined in my above comment, I can push to get that taken care of in the next couple days, or potentially stretching into Monday of next week.

[aaronsteers]

@cjohnhanson - Re:

These 7 tests can be resolved by addressing two underlying causes.

Shell environment variables take too high of precedence compared to what is described in the miro board, I'll create a follow-up issue for that which will resolve 6 of the currently `xfail'-ing tests.

Inheritance (e.g., setting a STACKED env var to 1 and then {STACKED}2, etc.) also basically doesn't work at all as expected. I'll create another follow-up issue to address that which will resolve the one remaining failing test case.

Thanks very much for this additional detail. I really like the approach of using xfail tests so we can keep those in the codebase and work those towards resolution.

So if I misunderstood scope here and we need to try to address one or both...

Actually, I think you've got the scope perfectly. If I understand correctly, there are basically three todos I'd ask before closing:

1. Create new issues for those 2 follow-on fixes as described above (and link them in a comment here for ref).
2. Can we add inline refs for each respective xfail tests, linking in a code comment to the logged issue that is expected to fix the failing test.
3. Not covered above, but I see failing tests still in pytest suite on GitLab. Is there additional work needed to get these to pass? https://gitlab.com/meltano/legacy-ci/meltano/-/jobs/2536112067

I think with these three items resolved, we can close. What I can't speak to yet is whether we'll move to prioritize either of the two new issues. I'll wait to review those more closely before I and @tayloramurphy try to make a call on relative priority. (If you feel like you can add a weight estimate on each, that could be very helpful.)

[aaronsteers]

Maybe more errors than this, but this is the first one I found:

https://gitlab.com/meltano/legacy-ci/meltano/-/jobs/2536112067#L917

>       assert len(result_no_ff.settings) == 11  # noqa: WPS432
E       assert 12 == 11

[cjohnhanson]

@aaronsteers there was some additional work needed to get these to pass -- I had introduced a regression in a recent commit.

Those specific failing tests are passing locally now. I'll check in on the pipeline later today and get those issues created and then add the inline refs next to the xfails, and then this should be good to merge+close today.

[cjohnhanson]

@aaronsteers - pipeline is now succeeding: https://gitlab.com/meltano/legacy-ci/meltano/-/pipelines/554154838

I've also created the issues for resolving xfail-ing tests and referenced them in comments in the test file: 17ce6c6

^That commit will have triggered a new pipeline run, but it only adds 4 lines of comments. I still need to add a short section about the env: key to the docs per @tayloramurphy's requested changes, and then this should be good to go.

[cjohnhanson]

@tayloramurphy is this what you had in mind for a quick blurb about env: key?
https://github.com/meltano/meltano/pull/5938/files#diff-627749b70e154d1698d041017804e255927fac4cac7383dd6ee3fd6fd05a3b70L128

[tayloramurphy]

@tayloramurphy is this what you had in mind for a quick blurb about env: key? https://github.com/meltano/meltano/pull/5938/files#diff-627749b70e154d1698d041017804e255927fac4cac7383dd6ee3fd6fd05a3b70L128

@cjohnhanson yep! That looks great - I have #5969 to track adding more 👍

[aaronsteers]

@cjohnhanson - re:

is this what you had in mind for a quick blurb about env: key?
https://github.com/meltano/meltano/pull/5938/files#diff-627749b70e154d1698d041017804e255927fac4cac7383dd6ee3fd6fd05a3b70L128

Love this! Thanks!