get_directory_pattern can falsely identify Windows machines as Mac / Linux

katieclaiborne-duet commented 1 month ago

Describe the bug

If a Windows user has set their HOME environment variable to use forward slashes, the get_directory_pattern macro will falsely identify their machine as having a Mac / Linux operating system.

As a result, int_all_graph_resources will derive directory and file paths incorrectly, and dependent rules like fct_source_directories and fct_model_directories will return test failures.

Steps to reproduce

Initialize a dbt project and install project evaluator package
On a Windows machine, set the HOME environment variable to use forward slashes
- Note: I don't have a Windows machine myself, but am opening this issue on behalf of a teammate who does.
Build project evaluator

Expected results

In int_all_graph_resources, I expected directory_path to be populated, and file_name to contain the name of the file following the last path separator.

Actual results

Instead, directory_path is a blank string, and file_name contains the unchanged file_path value.

Screenshots and log output

int_all_graph_resources

fct_model_directories

System information

The contents of your packages.yml file:

packages:
  - package: dbt-labs/audit_helper
    version: 0.12.0

  - package: dbt-labs/codegen
    version: 0.12.1

  - package: dbt-labs/dbt_external_tables
    version: 0.9.0

  - package: dbt-labs/dbt_project_evaluator
    version: 0.12.2

  - package: dbt-labs/dbt_utils
    version: 1.2.0

Which database are you using dbt with?

[ ] postgres
[ ] redshift
[x] bigquery
[ ] snowflake
[ ] trino/starburst
[ ] other (specify: ____)

The output of dbt --version:

Core:
  - installed: 1.8.5
  - latest:    1.8.5 - Up to date!

Plugins:
  - bigquery: 1.8.2 - Up to date!

Additional context

Are you interested in contributing the fix?

Yes, although I'd likely need some guidance!

b-per commented 1 month ago

Do you know why HOME has forward slashes in it? And does Windows accept this?

We only use Mac here, so it might be tough to test for a fix, but I am thinking that instead of checking the HOME env var, we could maybe check if in the graph the file paths contain /or \.

katieclaiborne-duet commented 1 month ago

Not sure of the why for this user, but it does appear that Windows can often accept either type of slash.

Checking the graph seems like a promising idea!

jeff-skoldberg-gmds commented 1 month ago

For me this issue is happening on Windows using Git Bash, which takes forward slashes for everything.

edit: I just tried in PowerShell and I still get rows for fct_source_directories for every model. So I guess Git Bash was not the cause.

edit 2: Snowflake

b-per commented 1 month ago

Could you try installing the package with the following syntax and see if it works?

packages:
  - git: "https://github.com/dbt-labs/dbt-project-evaluator.git"
    revision: "fix/os-identification"

If so, I can get it merged.

katieclaiborne-duet commented 1 month ago

We've confirmed that the fix works!

Big thanks to @aarpatt1 for testing on his Windows machine, which has a HOME environment variable that was previously returning a false (Mac) positive.

b-per commented 1 month ago

Great. I will check with the team to get it merged then.

dbt-labs / dbt-project-evaluator