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

fix: Avoid casting settings before env var expansion #7294

Merged
merged 3 commits into from
Feb 9, 2023

Conversation

WillDaSilva
Copy link
Member

This fixes a regression in #7232. In that PR logic was added to cast settings within the setting store. This was needed because some settings were not retrieved through a settings service in order to avoid infinite regression (i.e. the get method in a setting service cannot generally call itself).

A consequence of those changes was that now we are casting setting values before env var expansion has occurred. To address the regression, this PR makes the casting logic from #7232 disabled by default, and it is only enabled when accessing settings from within the get methods of a setting service (where we cannot rely on the regular setting casting).

A cleaner approach would be preferable, but this should be sufficient to address the regression while preserving the bug fix that #7232 implemented.

Closes #7293

@netlify
Copy link

netlify bot commented Feb 9, 2023

Deploy Preview for meltano canceled.

Name Link
🔨 Latest commit 17a08b1
🔍 Latest deploy log https://app.netlify.com/sites/meltano/deploys/63e561e46ca92000088c44cd

@WillDaSilva WillDaSilva marked this pull request as ready for review February 9, 2023 05:13
@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Merging #7294 (17a08b1) into main (11f948d) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7294      +/-   ##
==========================================
- Coverage   89.57%   89.56%   -0.01%     
==========================================
  Files         288      288              
  Lines       21469    21479      +10     
  Branches     2373     2373              
==========================================
+ Hits        19230    19237       +7     
- Misses       1874     1875       +1     
- Partials      365      367       +2     
Impacted Files Coverage Δ
src/meltano/core/settings_service.py 97.50% <100.00%> (ø)
src/meltano/core/settings_store.py 94.05% <100.00%> (-0.60%) ⬇️
...ests/meltano/core/test_project_settings_service.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@WillDaSilva
Copy link
Member Author

WillDaSilva commented Feb 9, 2023

The new test may be is flaky: https://github.com/meltano/meltano/actions/runs/4131188538/jobs/7138602617

I have yet to reproduce a failure like this.

EDIT: The flaky test was fixed in 17a08b1

@WillDaSilva WillDaSilva merged commit bcbc08a into main Feb 9, 2023
@WillDaSilva WillDaSilva deleted the 7293-env-var-config-cast-error branch February 9, 2023 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Cannot set integer settings with environment variables in meltano.yml
4 participants