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

Add logging for a file path that can't be cleaned #2060

Merged
merged 4 commits into from
Jan 27, 2020
Merged

Add logging for a file path that can't be cleaned #2060

merged 4 commits into from
Jan 27, 2020

Conversation

emilieschario
Copy link
Contributor

@emilieschario emilieschario commented Jan 18, 2020

Closes #2059

In a case where your dbt_project.yml file has:

source-paths: ["models"]

clean-targets:
  - "models"
  - "dbt_modules"

when you run dbt clean you'll see a message that looks like:

Checking models/*
Checking dbt_modules/*
 Cleaned dbt_modules/*

Because it doesn't tell you you did something wrong, you may not notice that the model/* file wasn't cleaned (rightfully so!). This MR proposes that the messaging be:

Checking model/*
model/* cannot be cleaned.
Checking dbt_modules/*
 Cleaned dbt_modules/*

No functional differences, just more details.

@cla-bot cla-bot bot added the cla:yes label Jan 18, 2020
@emilieschario
Copy link
Contributor Author

@drewbanin I think these tests are failing because someone internal has to kick them off

@drewbanin drewbanin changed the base branch from dev/0.15.1 to dev/0.15.2 January 21, 2020 00:49
@drewbanin
Copy link
Contributor

Thanks for making this PR @emilieschario! I just kicked off the tests here.

This is super minor, but what's your rationale behind not indenting the log line that you added here? And what would you think about updating this line to be even more apparent with something like:

 ERROR: not cleaning {}/* because it is protected

Let me know what you think!

@emilieschario
Copy link
Contributor Author

On the not indenting piece, it was being it was hard to anticipate the length, so it could be a short name like model or it could be kjdhfskjfdlakjldkj so

Checking model/*
kjdhfskjfdlakjldkj/* cannot be cleaned.
Checking dbt_modules/*
 Cleaned dbt_modules/*

But I love love love the error message! I'm going to update now so that you can kick off tests again, if you're still around

core/dbt/task/clean.py Outdated Show resolved Hide resolved
@emilieschario
Copy link
Contributor Author

Updated @drewbanin!

@drewbanin
Copy link
Contributor

Thanks @emilieschario! This LGTM, but I have terrible news: your new change has a line that's longer than 80 characters, so it won't pass our code formatting CI check.

Can you try changing this line:

                logger.info("ERROR: not cleaning {}/* because it is protected".format(path))

to look like:

                logger.info("ERROR: not cleaning {}/* because it is "
                            "protected".format(path))

This will fix the pep8 error that might show up in Circle momentarily :)

@emilieschario
Copy link
Contributor Author

Thanks for your patience here @drewbanin! Updated!

@drewbanin
Copy link
Contributor

np! Thanks for that - just kicked off the tests again and will get this merge when they pass :)

@drewbanin
Copy link
Contributor

drewbanin commented Jan 24, 2020

hey @emilieschario - this code is definitely read to roll, but in testing this feature, I did notice a big problem!

Check out this code (unrelated to your PR): https://github.com/fishtown-analytics/dbt/pull/2060/files#diff-0927dc49a0a3b3ad824b956c8f0abb3fR21-R26

On line 24 here, we call:

protected_abs_paths = [os.path.abspath for p in protected_paths]

The problem is that we're not calling the abspath function, so protected_abs_paths takes on a value like:

> protected_abs_paths
[<function abspath at 0x101f3e950>, <function abspath at 0x101f3e950>, <function abspath at 0x101f3e950>]

These list elements are pointers to the abspath method, not absolute paths themselves! As a result, the __is_protected_path method does not actually check if paths are protected :/

If you're interested in fixing that issue in the scope of this PR, I think that would be a really good and helpful change. If not, we can totally queue up a second ticket to address it.

Let me know!

@emilieschario
Copy link
Contributor Author

woah. so why didn't my file get cleaned before if it wasn't because it was being deemed a file path? how did I come to this conclusion? 😕

I think I got it. Thanks for giving me the chance to do that.

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

Thanks @emilieschario - looks great! I'm kicking off the tests one last time, then will merge this when they pass :)

@drewbanin drewbanin merged commit 73c418c into dbt-labs:dev/0.15.2 Jan 27, 2020
@emilieschario
Copy link
Contributor Author

Thanks for the opportunity @drewbanin!

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.

If you have a protected path in your clean targets, log that the file can't be cleaned.
2 participants