dbt-labs / dbt-labs/dbt

[CT-3508] Consolidate `system.rmdir` and `system.rmtree`

Open
#9,306 1 comment 0 reactions 0 assignees View on GitHub
engine:v1 type:tech-debt
Dominant language
Rust
Stars
13.8k
Forks
2.6k
Avg merge
21h 31m
Merged PRs (30d)
56

Description

### Housekeeping

- [X] I am a maintainer of dbt-core

### Short description

[`system.rmdir`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L223-L235) and [`system.rmtree`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L565-L570) look basically the same.

One uses [`_windows_rmdir_readonly`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L203-L209) and the other uses [`chmod_and_retry`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L508-L519) (which also look very similar).

Can we consolidate all of these for simplicity?

### Potential solution

It might be as simple as just replacing the implementation of one with a call to the other.

And then removing whichever of `_windows_rmdir_readonly` or `chmod_and_retry` is unused after that.

### Acceptance criteria

- There is no longer duplicated code within [`system.rmdir`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L223-L235) and [`system.rmtree`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L565-L570)
- There is no longer duplicated code within [`_windows_rmdir_readonly`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L203-L209) and [`chmod_and_retry`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L508-L519)

### Impact to Other Teams

No impact anticipated.

### Will backports be required?

No backports required.

### Context

Discovered while investigating https://github.com/dbt-labs/dbt-core/issues/9304.

#### `system.rmdir`

[`system.rmdir` ](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L223) was introduced in [#487](https://github.com/dbt-labs/dbt-core/pull/487/files#diff-20374548a72c09013ab95950949b7cd3d59f6d1a0faa406f231c9a6d2603322c) (2017-07-14) It was updated within [#542](https://github.com/dbt-labs/dbt-core/pull/542/files#diff-20374548a72c09013ab95950949b7cd3d59f6d1a0faa406f231c9a6d2603322c) (2018-02-27) to handle Windows files that git marks as read-only.

It is in use in these locations:
- https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/deps/git.py#L121
- https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/deps/local.py#L62
- https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/deps/tarball.py#L88

#### `system.rmtree`

[`system.rmtree`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L565) was added in [#1110](https://github.com/dbt-labs/dbt-core/pull/1110/files#diff-20374548a72c09013ab95950949b7cd3d59f6d1a0faa406f231c9a6d2603322c) (2018-11-14). It also handles read-only Windows files.

It is directly in use in these locations:
- https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/deps/base.py#L43
- https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/task/deps.py#L229
- https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L559

And it is _indirectly_ used (via [`system.move`](https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/clients/system.py#L526)) here:
- https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/deps/git.py#L123
- https://github.com/dbt-labs/dbt-core/blob/c2bc2f009bbeeb46b3c69d082ab4d485597898af/core/dbt/deps/tarball.py#L89

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.