kaxil commented on code in PR #71359:
URL: https://github.com/apache/airflow/pull/71359#discussion_r3753728101
##########
providers/anthropic/src/airflow/providers/anthropic/hooks/anthropic.py:
##########
@@ -309,6 +309,18 @@ def _workload_identity_credentials(wif: dict[str, Any]) ->
WorkloadIdentityCrede
Anthropic access token. See
https://platform.claude.com/docs/en/manage-claude/workload-identity-federation.
"""
+ required_fields = (
+ "identity_token_file",
+ "federation_rule_id",
+ "organization_id",
+ "service_account_id",
Review Comment:
`service_account_id` isn't required by the SDK:
`WorkloadIdentityCredentials.__init__` types it `Optional[str] = None` and only
adds it to the token-exchange body when it isn't None (same on 0.101.0, the
floor in `pyproject.toml`, and on current 0.120.2). The SDK's own env-driven
path gates on just `federation_rule_id` + `organization_id` and passes
`os.environ.get(ANTHROPIC_SERVICE_ACCOUNT_ID)` straight through, so
env-configured WIF works without it.
So this rejects a config the SDK accepts, and makes the
connection-configured path stricter than the env path documented right next to
it in `connections.rst`. Could it move into the optional block below alongside
`workspace_id`/`scope`? That would match how `aws_region` is handled
(`test_aws_platform_without_region`: pass None and let the SDK resolve). The
docstring on line 227 reads as required too, so worth fixing in the same pass.
##########
providers/anthropic/tests/unit/anthropic/hooks/test_anthropic.py:
##########
@@ -425,6 +425,41 @@ def test_workload_identity_federation_explicit(
)
mock_anthropic.assert_called_once_with(credentials=mock_wic.return_value,
base_url=None)
+ @pytest.mark.parametrize(
+ ("missing_field", "missing_value"),
+ [
+ pytest.param(field, value, id=f"{field}-{value or 'missing'}")
Review Comment:
`value or 'missing'` renders the same id for both cases, so pytest
disambiguates them as `identity_token_file-missing0` / `-missing1` and the
absent-vs-empty distinction disappears from the test output.
`id=f"{field}-{'absent' if value is None else 'empty'}"` reads better.
##########
providers/anthropic/src/airflow/providers/anthropic/hooks/anthropic.py:
##########
@@ -309,6 +309,18 @@ def _workload_identity_credentials(wif: dict[str, Any]) ->
WorkloadIdentityCrede
Anthropic access token. See
https://platform.claude.com/docs/en/manage-claude/workload-identity-federation.
"""
+ required_fields = (
+ "identity_token_file",
+ "federation_rule_id",
+ "organization_id",
+ "service_account_id",
+ )
+ missing_fields = [field for field in required_fields if not
wif.get(field)]
+ if missing_fields:
+ raise AnthropicError(
+ "The workload_identity configuration is missing required
fields: "
Review Comment:
`not wif.get(field)` also catches an empty string, so a key the user can see
sitting in their `extra` gets reported as "missing". "missing or empty required
fields" would match what the code does, and what the PR description says it
does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
