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

Support reading both yml and yaml file extensions for schema configuration files. #1681

Closed
soumyadsanyal opened this issue Aug 13, 2019 · 10 comments · Fixed by #2263
Closed
Labels
enhancement New feature or request good_first_issue Straightforward + self-contained changes, good for new contributors!

Comments

@soumyadsanyal
Copy link

Issue: Currently dbt checks checks for configuration files inside the models folder with the yml file extension on dbt run.

Problem: This may cause confusion for users that are used to using the official yaml file extension for YAML files.

Impact: configurations fail to load on dbt run, source block definitions aren't parsed, etc.

Proposed solution: support reading configuration files with both yml and yaml file extensions, or throw informative exceptions as desired in one case or the other.

cc @clrcrl

@drewbanin
Copy link
Contributor

Thanks @soumyadsanyal! I like the idea of including .yaml in addition to .yml.

I think the operative lines of code to change are around here. We may ultimately need to change the find_matching method to accept a list of file patterns instead of just accepting a single string.

Last: while this feels like a small change, it's probably something we'll want to do in a minor release (like 0.15.0). This could be a breaking change as dbt might start evaluating .yaml files that it had previously ignored (which could lead to compilation errors).

@sumanau7
Copy link
Contributor

sumanau7 commented Feb 19, 2020

@drewbanin Interested to pick this up if this is still a priority, as you have mentioned that this could be a breaking change, in order to maintain backward compatibility one of the ways we could proceed is as:

  1. User's can specify a flag in project settings on whether to enable all patterns for yml, sql or not, if flag is not present, the code will work the same way as its working now. Or they can specify the file extension themselves.
  2. In case flag is mentioned, we will include all forms of yaml, sql etc.. (caps or small).
  3. find_matching will take list as input instead of string.

@drewbanin
Copy link
Contributor

hey @sumanau7 - I'm not so concerned about making this a "breaking" change - it's just something I'd want to communicate at release-time.

I think we should do the following things:

  • make file name lookups case-insensitive (.sql, .SQL, .yml, .YML) all work
  • add .yaml as a recognized extension for schema files

via
https://github.com/fishtown-analytics/dbt/blob/b77b3a45661e68a647a6be91e197c1cd13545982/core/dbt/parser/schemas.py#L129-L132

The annoying part: I don't really want to support both .yml and .yaml for the core config files (dbt_project.yml, profiles.yml, packages.yml) as I think that would lead to a ton of user-space errors. So, I'm ok with adding as a historical footnote that fact that I picked the wrong extension (.yml instead of .yaml) back in 2016 and just live with the consequences :)

Let me know what you think!

@sumanau7
Copy link
Contributor

sumanau7 commented Mar 23, 2020

@drewbanin You are right, supporting both .yml and .yaml for the core config files may lead to user space errors.
IMO lets make file name lookups case-insensitive and throw a warning in case core config files are found with YAML extension explaining in order to support new standards, .YAML file will be read in future versions of dbt.

Let's say we receive a positive response and there are no new issues from user asking us to revert the change we can then make it live in the next release. I would say 3 months of warning period and then make the the new change live. (Supporting both .yml and .yaml)
thoughts ?

@drewbanin
Copy link
Contributor

Hey @sumanau7 - that sounds like a really good approach!

Let's go ahead and:

  1. make a PR to make file extensions case-insensitive
  2. search for .yaml files and raise a warning indicating that these files are not currently parsed by dbt, but will be in a future release

Really good thinking here! Are you interested in sending through a PR for this change? We'd be very helpful to assist you however we can if so :)

@drewbanin drewbanin modified the milestone: Octavius Catto Mar 26, 2020
@sumanau7
Copy link
Contributor

@drewbanin I will start work on this should be able to share a PR with TODO's and basic implementation over the weekend to get early feedback and then can take it forward.

@sumanau7
Copy link
Contributor

@drewbanin Does the first draft looks good ?

@Luttik
Copy link

Luttik commented Jun 22, 2021

This issue was closed but the pull request that was mentioned here didn't solve the issue. It only notified of a change to come. Does anybody know which pull request resolved the issue? Or was it never truly addressed?

@jtcohen6
Copy link
Contributor

@Luttik This was closed by #2263, which allowed for reading file extensions case-insensitively, and raised a warning if it found any with the .yaml extension. That change was included in v0.17.0 (May 2020), and included the code below:

https://github.com/fishtown-analytics/dbt/blob/97d63d3da4d89a138fad9ea7d0bbd366b740f40e/core/dbt/parser/schemas.py#L171-L179

Incidentally, that "future version" will actually be the next minor version of dbt (v0.20.0), currently in prerelease. This change is from #3244:

https://github.com/fishtown-analytics/dbt/blob/9d414f6ec30bfedbb448e8954a506f63d550dbad/core/dbt/parser/read_files.py#L135-L140

@Luttik
Copy link

Luttik commented Jun 22, 2021

@jtcohen6 Ah thanks I was looking through the pull request but I must have completely missed the changes to read_files.py...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good_first_issue Straightforward + self-contained changes, good for new contributors!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants