Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions ddpui/core/dbtfunctions.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,15 @@ def map_airbyte_destination_spec_to_dbtcli_profile(
if mode:
conn_info["sslmode"] = mode

if ca_certificate and dbt_project_params.org_project_dir:
file_path = os.path.join(dbt_project_params.org_project_dir, "sslrootcert.pem")
with open(file_path, "w", encoding="utf-8") as file:
file.write(ca_certificate)
conn_info["sslrootcert"] = file_path
if ca_certificate:
if not dbt_project_params or not dbt_project_params.org_project_dir:
raise Exception(
"org_project_dir is required to save the ca_certificate for dbt ssl connections"
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

conn_info["sslrootcert_content"] = ca_certificate
conn_info["sslrootcert"] = os.path.join(
dbt_project_params.org_project_dir, "sslrootcert.pem"
)
Comment thread
Ishankoradia marked this conversation as resolved.

return conn_info
2 changes: 1 addition & 1 deletion ddpui/ddpdbt/schema.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Union
from typing import Union, Optional
from ninja import Schema
from pathlib import Path

Expand Down
42 changes: 38 additions & 4 deletions ddpui/tests/core/test_dbtfunctions.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import os

from ddpui.ddpdbt.schema import DbtProjectParams
from ddpui.core.dbtfunctions import map_airbyte_destination_spec_to_dbtcli_profile

Expand Down Expand Up @@ -63,7 +65,7 @@ def test_map_airbyte_destination_spec_to_dbtcli_profile_success_tunnel_params(tm


def test_map_airbyte_destination_spec_to_dbtcli_profile_success_ssl_params(tmpdir):
"""Tests all the success cases"""
"""Tests ssl params are stored in conn_info for runtime cert writing"""
dbt_project_params = DbtProjectParams(
org_project_dir=str(tmpdir),
dbt_env_dir="/path/to/dbt_venv",
Expand All @@ -78,7 +80,39 @@ def test_map_airbyte_destination_spec_to_dbtcli_profile_success_ssl_params(tmpdi

conn_info = {"ssl_mode": {"mode": "verify-ca", "ca_certificate": "ca_certificate"}}
res = map_airbyte_destination_spec_to_dbtcli_profile(conn_info, dbt_project_params)
assert res["sslmode"] == conn_info["ssl_mode"]["mode"]
assert res["sslmode"] == "verify-ca"
assert res["sslrootcert"] == f"{tmpdir}/sslrootcert.pem"
with open(f"{tmpdir}/sslrootcert.pem") as file:
assert file.read() == conn_info["ssl_mode"]["ca_certificate"]
assert res["sslrootcert_content"] == "ca_certificate"
# cert should NOT be written to disk at setup time
assert not os.path.exists(f"{tmpdir}/sslrootcert.pem")


def test_map_airbyte_destination_spec_to_dbtcli_profile_ssl_mode_only(tmpdir):
"""Tests ssl_mode with mode but no ca_certificate"""
dbt_project_params = DbtProjectParams(
org_project_dir=str(tmpdir),
dbt_env_dir="/path/to/dbt_venv",
dbt_repo_dir="/path/to/dbt_repo",
project_dir="/path/to/project_dir",
target="target",
dbt_binary="dbt_binary",
venv_binary="path/to/venv/bin",
clients_base_dir="/path/to/clients_base",
project_dir_relative="org/dbtrepo",
)

conn_info = {"ssl_mode": {"mode": "require"}}
res = map_airbyte_destination_spec_to_dbtcli_profile(conn_info, dbt_project_params)
assert res["sslmode"] == "require"
assert "sslrootcert" not in res
assert "sslrootcert_content" not in res


def test_map_airbyte_destination_spec_to_dbtcli_profile_ssl_no_org_project_dir():
"""Tests ssl with ca_certificate but no dbt_project_params raises"""
conn_info = {"ssl_mode": {"mode": "verify-ca", "ca_certificate": "ca_certificate"}}
try:
map_airbyte_destination_spec_to_dbtcli_profile(conn_info, None)
assert False, "should have raised"
except Exception as e:
assert "org_project_dir is required" in str(e)
Comment on lines +111 to +118
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Replace try/except + assert False with pytest.raises.

Two Ruff-flagged problems here:

  1. B011assert False is silently removed under python -O, so the "should have raised" sentinel never fires in optimised mode.
  2. BLE001 — bare except Exception swallows any unexpected error (AttributeError, KeyError, etc.) and checks its message against "org_project_dir is required", meaning an unrelated crash causes a false-pass.

pytest.raises fixes both in one move:

🛠️ Proposed fix
+import pytest
+
 def test_map_airbyte_destination_spec_to_dbtcli_profile_ssl_no_org_project_dir():
     """Tests ssl with ca_certificate but no dbt_project_params raises"""
     conn_info = {"ssl_mode": {"mode": "verify-ca", "ca_certificate": "ca_certificate"}}
-    try:
-        map_airbyte_destination_spec_to_dbtcli_profile(conn_info, None)
-        assert False, "should have raised"
-    except Exception as e:
-        assert "org_project_dir is required" in str(e)
+    with pytest.raises(Exception, match="org_project_dir is required"):
+        map_airbyte_destination_spec_to_dbtcli_profile(conn_info, None)
🧰 Tools
🪛 Ruff (0.15.12)

[warning] 116-116: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)


[warning] 117-117: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ddpui/tests/core/test_dbtfunctions.py` around lines 111 - 118, Replace the
try/except + assert False pattern in
test_map_airbyte_destination_spec_to_dbtcli_profile_ssl_no_org_project_dir with
pytest.raises to avoid B011 and BLE001: wrap the call to
map_airbyte_destination_spec_to_dbtcli_profile(conn_info, None) in a with
pytest.raises(<expected-exception-type>, match="org_project_dir is required")
block (use the concrete exception that the function raises, e.g., ValueError) so
the test asserts the specific exception and message instead of catching all
exceptions or relying on assert False.

Loading