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

Updated injected_node.wrapped_sql for data tests #1688

Merged
merged 7 commits into from
Aug 20, 2019
Merged

Updated injected_node.wrapped_sql for data tests #1688

merged 7 commits into from
Aug 20, 2019

Conversation

mikaelene
Copy link
Contributor

The data tests feature uses in-code-sql that does not support all databases. This is a fix for supporting more databases and custom adapters.

This should not effect currently supported databases in any way.

ref conversation on dbt slack: https://getdbt.slack.com/archives/C50NEBJGG/p1565962661033700

@cla-bot
Copy link

cla-bot bot commented Aug 17, 2019

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @mikaelene

@drewbanin
Copy link
Contributor

Hey @mikaelene, thanks for the PR! I think we'll be able to slip this in for the 0.14.1 release. Let me kick off the tests here - this will be ready to merge when they pass.

Thanks also for filling out the CLA. You're the first person to make a PR since we added it - glad to see it's working well :D

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Aug 17, 2019

The cla-bot has been summoned, and re-checked this pull request!

@cla-bot cla-bot bot added the cla:yes label Aug 17, 2019
@drewbanin drewbanin added this to the 0.14.1 milestone Aug 17, 2019
@drewbanin
Copy link
Contributor

Looks like there's a formatting issue here:

core/dbt/compilation.py:137:80: E501 line too long (82 > 79 characters)

You can run

docker-compose run test tox -e flake8

locally to run the flake8 linter. I'll kick these tests off again once that's updated!

@cla-bot
Copy link

cla-bot bot commented Aug 19, 2019

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: mikael.ene.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot removed the cla:yes label Aug 19, 2019
@mikaelene
Copy link
Contributor Author

Hi, I've updated now to support flake8

@mikaelene
Copy link
Contributor Author

I am using another pc today and the cla-bot apparently didn't get my cla-email. I've updated my git-profile. Do I need to do a "dummy"-commit or something?

@drewbanin
Copy link
Contributor

Thanks @mikaelene! You can ignore the cla issue - I see your signature... will check out why this bot is misbehaving offline.

Just kicked the tests off - will merge this when they pass :)

@drewbanin
Copy link
Contributor

oops, looks like flake8 is still complaining, try changing the code to something like:

                injected_node.wrapped_sql = (
                    "select count(*) as errors "
                    "from (\n{test_sql}\n) sbq").format(
                        test_sql=injected_node.injected_sql)

@drewbanin
Copy link
Contributor

hey @mikaelene - are you using Windows? It looks like every line in this file has changed - is that a line ending thing?

@mikaelene
Copy link
Contributor Author

Crap!

I didn't see that. I am using both windows and mac and store my project on Dropbox. That might be the problem. Let me try again.

@mikaelene
Copy link
Contributor Author

Looks better now right?

@drewbanin
Copy link
Contributor

I'm still seeing +252 -251 :/

Screen Shot 2019-08-20 at 10 07 28 AM

@mikaelene
Copy link
Contributor Author

Still learning to git :-(. How about now?

@drewbanin
Copy link
Contributor

Looks great, thanks @mikaelene! Kicking off the tests here - will merge this when they pass :)

@drewbanin
Copy link
Contributor

merging! Thanks for your contribution @mikaelene :)

@drewbanin drewbanin merged commit 0e897f7 into dbt-labs:dev/0.14.1 Aug 20, 2019
@mikaelene
Copy link
Contributor Author

Wow. Finally got it right. :-)

@mikaelene mikaelene deleted the project_cols_data_test branch August 20, 2019 19:02
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.

2 participants