fix: address coderabbit review findings
This commit is contained in:
@@ -267,7 +267,7 @@ def _rpc_timeout_from_optional_config(
|
|||||||
return override
|
return override
|
||||||
try:
|
try:
|
||||||
config = load_workflow_config(path)
|
config = load_workflow_config(path)
|
||||||
except FileNotFoundError, json.JSONDecodeError, ValidationError:
|
except (FileNotFoundError, json.JSONDecodeError, ValidationError):
|
||||||
return 30.0
|
return 30.0
|
||||||
target = config.client.target
|
target = config.client.target
|
||||||
if isinstance(target, RpcHttpTargetConfig):
|
if isinstance(target, RpcHttpTargetConfig):
|
||||||
|
|||||||
+2
-2
@@ -112,7 +112,7 @@ def auth_missing_diagnostic(
|
|||||||
def connection_auth_diagnostic(
|
def connection_auth_diagnostic(
|
||||||
connection: ConnectionConfig,
|
connection: ConnectionConfig,
|
||||||
*,
|
*,
|
||||||
load_auth: Callable[[str], AuthRecord | None],
|
load_auth_ref: Callable[[str], AuthRecord | None],
|
||||||
logical_ref: str | None = None,
|
logical_ref: str | None = None,
|
||||||
) -> DependencyDiagnostic | None:
|
) -> DependencyDiagnostic | None:
|
||||||
"""Return an auth diagnostic for explicit auth_ref misses.
|
"""Return an auth diagnostic for explicit auth_ref misses.
|
||||||
@@ -125,7 +125,7 @@ def connection_auth_diagnostic(
|
|||||||
auth_ref = auth_ref_for_connection(connection)
|
auth_ref = auth_ref_for_connection(connection)
|
||||||
if auth_ref is None:
|
if auth_ref is None:
|
||||||
return None
|
return None
|
||||||
if load_auth(auth_ref) is not None:
|
if load_auth_ref(auth_ref) is not None:
|
||||||
return None
|
return None
|
||||||
return auth_missing_diagnostic(
|
return auth_missing_diagnostic(
|
||||||
auth_ref=auth_ref,
|
auth_ref=auth_ref,
|
||||||
|
|||||||
@@ -164,7 +164,7 @@ class SourceRegistryAdminProvider(WorkflowSourceRegistryMutationProvider):
|
|||||||
for source_id in sorted(after):
|
for source_id in sorted(after):
|
||||||
diagnostic = connection_auth_diagnostic(
|
diagnostic = connection_auth_diagnostic(
|
||||||
after[source_id],
|
after[source_id],
|
||||||
load_auth=self.load_auth,
|
load_auth_ref=self.load_auth,
|
||||||
)
|
)
|
||||||
if diagnostic is not None:
|
if diagnostic is not None:
|
||||||
auth_diagnostics.append(diagnostic.model_dump(mode="json"))
|
auth_diagnostics.append(diagnostic.model_dump(mode="json"))
|
||||||
|
|||||||
@@ -312,7 +312,9 @@ class UpstreamTransportService:
|
|||||||
continue
|
continue
|
||||||
auth_diagnostic = connection_auth_diagnostic(
|
auth_diagnostic = connection_auth_diagnostic(
|
||||||
connection,
|
connection,
|
||||||
load_auth=self.load_auth,
|
# The diagnostic helper passes the explicit auth_ref to this
|
||||||
|
# loader, matching load_connection_auth's auth_ref-first path.
|
||||||
|
load_auth_ref=self.load_auth,
|
||||||
logical_ref=logical_ref,
|
logical_ref=logical_ref,
|
||||||
)
|
)
|
||||||
if auth_diagnostic is not None:
|
if auth_diagnostic is not None:
|
||||||
|
|||||||
@@ -131,8 +131,8 @@ def _api(auth=None) -> WorkflowAdminApi:
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_admin_lists_auth_records_sorted_without_payload_values() -> None:
|
async def test_admin_lists_auth_records_sorted_without_payload_values() -> None:
|
||||||
payload = asyncio.run(_api(AuthProvider()).list_auth_records())
|
payload = await _api(AuthProvider()).list_auth_records()
|
||||||
|
|
||||||
assert payload["total"] == 2
|
assert payload["total"] == 2
|
||||||
assert [record["id"] for record in payload["auth_records"]] == [
|
assert [record["id"] for record in payload["auth_records"]] == [
|
||||||
@@ -143,8 +143,8 @@ def test_admin_lists_auth_records_sorted_without_payload_values() -> None:
|
|||||||
assert "payload" not in payload["auth_records"][0]
|
assert "payload" not in payload["auth_records"][0]
|
||||||
|
|
||||||
|
|
||||||
def test_admin_inspects_auth_record_without_payload_values() -> None:
|
async def test_admin_inspects_auth_record_without_payload_values() -> None:
|
||||||
payload = asyncio.run(_api(AuthProvider()).inspect_auth_record("github.work"))
|
payload = await _api(AuthProvider()).inspect_auth_record("github.work")
|
||||||
|
|
||||||
assert payload == {
|
assert payload == {
|
||||||
"id": "github.work",
|
"id": "github.work",
|
||||||
@@ -154,12 +154,12 @@ def test_admin_inspects_auth_record_without_payload_values() -> None:
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
def test_admin_auth_methods_report_unavailable_without_provider() -> None:
|
async def test_admin_auth_methods_report_unavailable_without_provider() -> None:
|
||||||
with pytest.raises(RuntimeError, match="auth admin is not available"):
|
with pytest.raises(RuntimeError, match="auth admin is not available"):
|
||||||
asyncio.run(_api().list_auth_records())
|
await _api().list_auth_records()
|
||||||
|
|
||||||
with pytest.raises(RuntimeError, match="auth admin is not available"):
|
with pytest.raises(RuntimeError, match="auth admin is not available"):
|
||||||
asyncio.run(_api().inspect_auth_record("github.work"))
|
await _api().inspect_auth_record("github.work")
|
||||||
|
|
||||||
|
|
||||||
class MutableAuthProvider(AuthProvider):
|
class MutableAuthProvider(AuthProvider):
|
||||||
@@ -191,18 +191,16 @@ class MutableAuthProvider(AuthProvider):
|
|||||||
return {"deleted": True, "id": auth_ref}
|
return {"deleted": True, "id": auth_ref}
|
||||||
|
|
||||||
|
|
||||||
def test_admin_saves_auth_record_without_payload_values() -> None:
|
async def test_admin_saves_auth_record_without_payload_values() -> None:
|
||||||
provider = MutableAuthProvider()
|
provider = MutableAuthProvider()
|
||||||
api = _api(provider)
|
api = _api(provider)
|
||||||
|
|
||||||
payload = asyncio.run(
|
payload = await api.save_auth_record(
|
||||||
api.save_auth_record(
|
|
||||||
auth_ref="drive.work",
|
auth_ref="drive.work",
|
||||||
scheme="bearer",
|
scheme="bearer",
|
||||||
payload={"token": "secret"},
|
payload={"token": "secret"},
|
||||||
metadata={"owner": "test"},
|
metadata={"owner": "test"},
|
||||||
)
|
)
|
||||||
)
|
|
||||||
|
|
||||||
assert payload == {
|
assert payload == {
|
||||||
"id": "drive.work",
|
"id": "drive.work",
|
||||||
@@ -213,33 +211,29 @@ def test_admin_saves_auth_record_without_payload_values() -> None:
|
|||||||
assert "secret" not in str(payload)
|
assert "secret" not in str(payload)
|
||||||
|
|
||||||
|
|
||||||
def test_admin_deletes_auth_record() -> None:
|
async def test_admin_deletes_auth_record() -> None:
|
||||||
provider = MutableAuthProvider()
|
provider = MutableAuthProvider()
|
||||||
api = _api(provider)
|
api = _api(provider)
|
||||||
asyncio.run(
|
await api.save_auth_record(
|
||||||
api.save_auth_record(
|
|
||||||
auth_ref="drive.work",
|
auth_ref="drive.work",
|
||||||
scheme="bearer",
|
scheme="bearer",
|
||||||
payload={"token": "secret"},
|
payload={"token": "secret"},
|
||||||
)
|
)
|
||||||
)
|
|
||||||
|
|
||||||
payload = asyncio.run(api.delete_auth_record("drive.work"))
|
payload = await api.delete_auth_record("drive.work")
|
||||||
|
|
||||||
assert payload == {"deleted": True, "id": "drive.work"}
|
assert payload == {"deleted": True, "id": "drive.work"}
|
||||||
with pytest.raises(KeyError):
|
with pytest.raises(KeyError):
|
||||||
provider.inspect_auth_record("drive.work")
|
provider.inspect_auth_record("drive.work")
|
||||||
|
|
||||||
|
|
||||||
def test_admin_auth_mutations_report_unavailable_without_provider() -> None:
|
async def test_admin_auth_mutations_report_unavailable_without_provider() -> None:
|
||||||
with pytest.raises(RuntimeError, match="auth admin is not available"):
|
with pytest.raises(RuntimeError, match="auth admin is not available"):
|
||||||
asyncio.run(
|
await _api().save_auth_record(
|
||||||
_api().save_auth_record(
|
|
||||||
auth_ref="drive.work",
|
auth_ref="drive.work",
|
||||||
scheme="bearer",
|
scheme="bearer",
|
||||||
payload={"token": "secret"},
|
payload={"token": "secret"},
|
||||||
)
|
)
|
||||||
)
|
|
||||||
|
|
||||||
with pytest.raises(RuntimeError, match="auth admin is not available"):
|
with pytest.raises(RuntimeError, match="auth admin is not available"):
|
||||||
asyncio.run(_api().delete_auth_record("drive.work"))
|
await _api().delete_auth_record("drive.work")
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import shutil
|
import shutil
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
from wf_authoring import NodeSpec
|
from wf_authoring import NodeSpec
|
||||||
from wf_core import RunStatus
|
from wf_core import RunStatus
|
||||||
@@ -502,7 +503,7 @@ def test_source_catalog_finds_local_documentation_resource_directly() -> None:
|
|||||||
assert result.uri == test_resource.uri
|
assert result.uri == test_resource.uri
|
||||||
|
|
||||||
|
|
||||||
def test_source_catalog_uses_catalog_store_only(tmp_path) -> None:
|
def test_source_catalog_uses_catalog_store_only(tmp_path: Path) -> None:
|
||||||
catalog_store = FileCatalogStore(tmp_path / "catalog")
|
catalog_store = FileCatalogStore(tmp_path / "catalog")
|
||||||
service = SourceCatalogService(
|
service = SourceCatalogService(
|
||||||
store=catalog_store,
|
store=catalog_store,
|
||||||
|
|||||||
@@ -311,7 +311,9 @@ async def test_upstream_transport_live_diagnostics_report_missing_auth_ref(
|
|||||||
assert "github.creds" in diagnostics[0].message
|
assert "github.creds" in diagnostics[0].message
|
||||||
|
|
||||||
|
|
||||||
def test_upstream_transport_uses_separate_auth_and_catalog_stores(tmp_path) -> None:
|
def test_upstream_transport_uses_separate_auth_and_catalog_stores(
|
||||||
|
tmp_path: Path,
|
||||||
|
) -> None:
|
||||||
auth_store = FileAuthStore(tmp_path / "auth")
|
auth_store = FileAuthStore(tmp_path / "auth")
|
||||||
catalog_store = FileCatalogStore(tmp_path / "catalog")
|
catalog_store = FileCatalogStore(tmp_path / "catalog")
|
||||||
events = []
|
events = []
|
||||||
|
|||||||
@@ -157,7 +157,7 @@ def test_connection_auth_diagnostic_reports_missing_auth_ref() -> None:
|
|||||||
|
|
||||||
diagnostic = connection_auth_diagnostic(
|
diagnostic = connection_auth_diagnostic(
|
||||||
connection,
|
connection,
|
||||||
load_auth=lambda auth_ref: None,
|
load_auth_ref=lambda auth_ref: None,
|
||||||
logical_ref="github",
|
logical_ref="github",
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -188,7 +188,7 @@ def test_connection_auth_diagnostic_ignores_absent_or_present_auth_ref() -> None
|
|||||||
assert (
|
assert (
|
||||||
connection_auth_diagnostic(
|
connection_auth_diagnostic(
|
||||||
no_ref,
|
no_ref,
|
||||||
load_auth=lambda auth_ref: None,
|
load_auth_ref=lambda auth_ref: None,
|
||||||
logical_ref="github",
|
logical_ref="github",
|
||||||
)
|
)
|
||||||
is None
|
is None
|
||||||
@@ -196,7 +196,7 @@ def test_connection_auth_diagnostic_ignores_absent_or_present_auth_ref() -> None
|
|||||||
assert (
|
assert (
|
||||||
connection_auth_diagnostic(
|
connection_auth_diagnostic(
|
||||||
with_ref,
|
with_ref,
|
||||||
load_auth=lambda auth_ref: auth,
|
load_auth_ref=lambda auth_ref: auth,
|
||||||
logical_ref="github",
|
logical_ref="github",
|
||||||
)
|
)
|
||||||
is None
|
is None
|
||||||
|
|||||||
Reference in New Issue
Block a user