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

Do not cast unscrubbed values to a string #165

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Conversation

dbeatty10
Copy link
Contributor

@dbeatty10 dbeatty10 commented Jul 10, 2024

resolves dbt-labs/dbt-core#10421

Problem

The scrub_secrets method casts all input to str systematically, and dbt-labs/dbt-core#9733 applies the scrubbing function to all CLI variables.

This can create issues during dbt retry because all values other than str will have their data type changed.

Solution

Leave values as-is during the scrubbing process unless they contain a secret that is scrubbed out.

Checklist

@dbeatty10 dbeatty10 requested a review from a team as a code owner July 10, 2024 01:07
@cla-bot cla-bot bot added the cla:yes label Jul 10, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.35%. Comparing base (e1d77b6) to head (e87dd64).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #165   +/-   ##
=======================================
  Coverage   67.35%   67.35%           
=======================================
  Files          52       52           
  Lines        3370     3370           
=======================================
  Hits         2270     2270           
  Misses       1100     1100           
Flag Coverage Δ
unit 67.35% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbeatty10
Copy link
Contributor Author

Thanks for taking a look at this @MichelleArk 🙏

Does the CI tests include end-to-end testing with dbt-core and dbt-postgres ? I didn't do any testing of this proposed change, so it would be good to know that existing functional tests in core and adapters still work after this change.

@MichelleArk
Copy link
Collaborator

It does not. dbt-core tests will run once this is on the merge queue which use dbt-postgres. The dbt-postgres integration tests are currently largely a mirror of what's in dbt-core so I don't think we'd get tons more signal by just running those.

@mmansikka
Copy link

Finally this is solved! Any update on when this PR is merged?

@dbeatty10 dbeatty10 enabled auto-merge September 20, 2024 15:52
@dbeatty10 dbeatty10 added this pull request to the merge queue Sep 20, 2024
Merged via the queue into main with commit 98ccfce Sep 20, 2024
15 checks passed
@dbeatty10 dbeatty10 deleted the dbeatty/fix-10421 branch September 20, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Regression] all --vars of run_results.json artifact are casted to str but they were not before
4 participants