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.

Sign up for GitHub

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

Jump to bottom

[#3885] Handle env_vars in partial parsing of SQL files #4101

Merged
merged 2 commits into from
Oct 26, 2021
Merged

[#3885] Handle env_vars in partial parsing of SQL files #4101

merged 2 commits into from
Oct 26, 2021

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Oct 20, 2021

resolves #3885

Description

This code collects the env_vars from contexts that inherit from ProviderContext (SQL files), stores the env_var values in the manifest, and in the partial parsing code schedules the files with changed env_vars for parsing.

In addition, this makes the function names that generate contexts consistent (generate_[name]_context) and changes the 'created_at' field in various nodes to a float (time.time()) instead of int for more precision, because it was needed in a test case. Also removed some unnecessary code from the schema parser that handled pre-v2 configs.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@cla-bot cla-bot bot added the cla:yes label Oct 20, 2021
jtcohen6
jtcohen6 reviewed Oct 21, 2021
View reviewed changes
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In basic local testing, this works wonderfully for .sql files. I left one comment around handling secret-prefixed env vars.

I'm really excited to extend this to .yml files, and eventually to work with var() as well as env_var().

core/dbt/parser/partial.py
@@ -790,3 +798,28 @@ def remove_source_override_target(self, source_dict):
self.remove_tests(orig_file, 'sources', orig_source['name'])
self.merge_patch(orig_file, 'sources', orig_source)
self.add_to_pp_files(orig_file)

def build_env_vars_to_source_files(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method where we should check to see if env vars start with SECRET_ENV_PREFIX, and (if so) not store their value / always invalidate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using the stored env_var values, so probably that needs to happen earlier in the provider context env_var method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a chance to discuss this offline, and arrived at: we should always store ***** as the value for secret env vars, i.e., neither store their values nor invalidate/re-parse files if the (unknown) value changes. This should work well with the intended use case for secret-prefixed env vars (packages.yml), or anticipated use in profiles.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was slightly simpler to just not store env vars starting with the secret prefix, because otherwise we have to convert when we read them in the partial parser too. Does this work for you?

@gshank gshank force-pushed the pp_sql_env_vars branch 2 times, most recently from 5e02006 to 54f2e55 Compare October 21, 2021 19:48
nathaniel-may
nathaniel-may approved these changes Oct 25, 2021
View reviewed changes
Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only some minor suggestions.

test/integration/068_partial_parsing_tests/test_pp_vars.py



class EnvVarTest(BasePPTest):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test! Very easy to follow.

test/integration/068_partial_parsing_tests/test_pp_vars.py Show resolved Hide resolved
core/dbt/parser/partial.py Show resolved Hide resolved
@gshank gshank merged commit 73af9a5 into main Oct 26, 2021
@gshank gshank deleted the pp_sql_env_vars branch October 26, 2021 15:16
jtcohen6 added a commit to dbt-labs/dbt-redshift that referenced this pull request Oct 27, 2021
@jtcohen6
Methods renamed in dbt-labs/dbt-core#4101
d359510
jtcohen6 added a commit to dbt-labs/dbt-redshift that referenced this pull request Oct 27, 2021
@jtcohen6
Fix tests for dbt-labs/dbt-core#3955 (#32)
2769ae5 
* Fix tests for dbt-labs/dbt-core#3955

* Methods renamed in dbt-labs/dbt-core#4101

* Missed one
@jtcohen6 jtcohen6 mentioned this pull request Oct 29, 2021
Closed
1 task
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@gshank @iknox-fa
[#3885] Handle env_vars in partial parsing of SQL files (#4101)
c134a08 
* [#3885] Handle env_vars in partial parsing

* Comment method to build env_vars_to_source_files

automatic commit by git-black, original commits:
  73af9a5
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@gshank @iknox-fa
[#3885] Handle env_vars in partial parsing of SQL files (#4101)
3fa31d7 
* [#3885] Handle env_vars in partial parsing

* Comment method to build env_vars_to_source_files

automatic commit by git-black, original commits:
  73af9a5
  dc65118
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@gshank @iknox-fa
[#3885] Handle env_vars in partial parsing of SQL files (#4101)
dbb8d1b 
* [#3885] Handle env_vars in partial parsing

* Comment method to build env_vars_to_source_files

automatic commit by git-black, original commits:
  307d47e
  73af9a5
  e2cd45b
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@gshank @iknox-fa
[#3885] Handle env_vars in partial parsing of SQL files (#4101)
cfd02af 
* [#3885] Handle env_vars in partial parsing

* Comment method to build env_vars_to_source_files

automatic commit by git-black, original commits:
  22db36c
  307d47e
  73af9a5
  e3fb923
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@gshank @iknox-fa
[#3885] Handle env_vars in partial parsing of SQL files (#4101)
0d834a5 
* [#3885] Handle env_vars in partial parsing

* Comment method to build env_vars_to_source_files

automatic commit by git-black, original commits:
  32c5598
  73af9a5
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@gshank @iknox-fa
[#3885] Handle env_vars in partial parsing of SQL files (#4101)
d7ec2d1 
* [#3885] Handle env_vars in partial parsing

* Comment method to build env_vars_to_source_files

automatic commit by git-black, original commits:
  73af9a5
  a5ce658
abbywh pushed a commit to abbywh/dbt-redshift that referenced this pull request Oct 11, 2023
@jtcohen6
Fix tests for dbt-labs/dbt-core#3955 (dbt-labs#32)
d671d3a 
* Fix tests for dbt-labs/dbt-core#3955

* Methods renamed in dbt-labs/dbt-core#4101

* Missed one
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtcohen6 jtcohen6 jtcohen6 left review comments

@nathaniel-may nathaniel-may nathaniel-may approved these changes

@leahwicz leahwicz Awaiting requested review from leahwicz

@emmyoop emmyoop Awaiting requested review from emmyoop

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Env vars + partial parsing
3 participants
@gshank @nathaniel-may @jtcohen6