Skip to content

Commit 66dceae

Browse files
committed
fix(sdk-review): apply_label idempotent — no-op when correct label already set
Each re-run was doing remove+add even when the label didn't change, creating a 'added X and removed X' churn event in the PR timeline for every execution. Now reads the current sdk-review label first: if it matches the target, returns immediately. Only removes labels that differ from the target.
1 parent 555ebbf commit 66dceae

9 files changed

Lines changed: 469 additions & 358 deletions

File tree

.claude/scripts/lib/github-api.sh

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -225,19 +225,29 @@ apply_label() {
225225
gh_api "repos/${owner_repo}/labels" -F name="$name" -F color="$color" > /dev/null 2>&1 || true
226226
done
227227

228-
# remove any existing sdk-review labels first
229-
# Guard: `grep` returns 1 on no-match, which combined with `set -o pipefail`
230-
# aborts the caller. Use `|| true` to swallow that.
228+
# Idempotent: read current labels first. If the target label is already
229+
# the ONLY sdk-review label, skip all GitHub API calls to avoid the noisy
230+
# "added X and removed X" churn in the PR timeline.
231231
local existing_labels
232232
existing_labels=$(gh pr view "$pr" --json labels -q '.labels[].name' 2>/dev/null || echo "")
233-
echo "$existing_labels" | grep '^sdk-review:' 2>/dev/null | while read -r existing; do
234-
[ -n "$existing" ] && gh pr edit "$pr" --remove-label "$existing" > /dev/null 2>&1 || true
233+
local current_sdk_labels
234+
current_sdk_labels=$(echo "$existing_labels" | grep '^sdk-review:' 2>/dev/null || true)
235+
if [ "$current_sdk_labels" = "$label" ]; then
236+
return 0 # already correct — nothing to do
237+
fi
238+
239+
# remove any existing sdk-review labels that differ from target
240+
echo "$current_sdk_labels" | while read -r existing; do
241+
[ -n "$existing" ] && [ "$existing" != "$label" ] && \
242+
gh pr edit "$pr" --remove-label "$existing" > /dev/null 2>&1 || true
235243
done || true
236244

237-
# add the target label
238-
gh pr edit "$pr" --add-label "$label" > /dev/null 2>&1 || {
239-
echo "WARN: could not apply label (fork PR or missing pull-requests:write scope)" >&2
240-
}
245+
# add the target label only if not already present
246+
if ! echo "$current_sdk_labels" | grep -Fxq "$label" 2>/dev/null; then
247+
gh pr edit "$pr" --add-label "$label" > /dev/null 2>&1 || {
248+
echo "WARN: could not apply label (fork PR or missing pull-requests:write scope)" >&2
249+
}
250+
fi
241251
}
242252

243253
if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
Lines changed: 52 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,64 @@
11
"""
2-
Secret resolver: load configuration/secrets from mounted files or environment variables
2+
Secret resolver: load configuration/secrets from mounted files or environment variables.
33
4-
Usage:
5-
from dataclasses import dataclass, field
6-
from sap_cloud_sdk.secret_resolver import read_from_mount_and_fallback_to_env_var
4+
Built-in resolvers and chain builder::
75
8-
@dataclass
9-
class MyConfig:
10-
username: str = field(metadata={"secret": "username"})
11-
password: str = field(metadata={"secret": "password"})
12-
endpoint: str = "http://localhost"
6+
from sap_cloud_sdk.core.secret_resolver import (
7+
MountResolver,
8+
EnvVarResolver,
9+
ChainedResolver,
10+
)
11+
12+
# Build a chain explicitly
13+
resolver = ChainedResolver([MountResolver(), EnvVarResolver()])
14+
resolver.resolve("destination", "default", binding)
15+
16+
Legacy function-based API (still supported)::
17+
18+
from sap_cloud_sdk.core.secret_resolver import read_from_mount_and_fallback_to_env_var
1319
14-
cfg = MyConfig()
1520
read_from_mount_and_fallback_to_env_var(
1621
base_volume_mount="/etc/secrets/appfnd",
1722
base_var_name="CLOUD_SDK_CFG",
18-
module="objectstore",
23+
module="destination",
1924
instance="default",
20-
target=cfg
25+
target=binding,
2126
)
2227
"""
2328

24-
from .resolver import read_from_mount_and_fallback_to_env_var, resolve_base_mount
29+
from sap_cloud_sdk.core.secret_resolver.resolver import (
30+
read_from_mount_and_fallback_to_env_var,
31+
)
32+
from sap_cloud_sdk.core.secret_resolver._resolvers import (
33+
Resolver,
34+
ChainedResolver,
35+
)
36+
37+
from sap_cloud_sdk.core.secret_resolver.mount_resolver import (
38+
MountResolver,
39+
resolve_base_mount,
40+
)
41+
from sap_cloud_sdk.core.secret_resolver.env_resolver import EnvVarResolver
42+
43+
from sap_cloud_sdk.core.secret_resolver.sdk_config import (
44+
SdkConfig,
45+
configure,
46+
get_sdk_config,
47+
get_resolver,
48+
)
2549

26-
__all__ = ["read_from_mount_and_fallback_to_env_var", "resolve_base_mount"]
50+
__all__ = [
51+
# Class-based API
52+
"Resolver",
53+
"MountResolver",
54+
"EnvVarResolver",
55+
"ChainedResolver",
56+
# Global configuration
57+
"SdkConfig",
58+
"configure",
59+
"get_sdk_config",
60+
"get_resolver",
61+
# Legacy function-based API
62+
"read_from_mount_and_fallback_to_env_var",
63+
"resolve_base_mount",
64+
]
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
"""Utilities for mapping dataclass fields to secret store keys."""
2+
3+
from typing import Any, Dict, Tuple
4+
from dataclasses import fields, is_dataclass
5+
6+
7+
def _get_field_map(target: Any) -> dict[str, tuple[str, type]]:
8+
"""
9+
Build a mapping from secret key -> (attribute_name, attribute_type) for a dataclass instance.
10+
11+
Priority:
12+
1. Use field.metadata["secret"] if present as the key
13+
2. Fallback to the lowercase dataclass field name
14+
Only string-typed fields are supported.
15+
"""
16+
if not is_dataclass(target) or isinstance(target, type):
17+
raise TypeError("target must be a dataclass instance")
18+
19+
mapping: Dict[str, Tuple[str, type]] = {}
20+
for f in fields(target):
21+
# Only support string fields for secrets (consistent with Go SDK)
22+
# Allow plain 'str' annotations; reject others to keep behavior predictable
23+
if f.type is not str:
24+
raise TypeError(
25+
f"target field '{f.name}' is not a string (only str fields are supported)"
26+
)
27+
key = f.metadata.get("secret") if hasattr(f, "metadata") else None
28+
if key and isinstance(key, str) and key.strip():
29+
mapping[key] = (f.name, f.type)
30+
else:
31+
mapping[f.name.lower()] = (f.name, f.type)
32+
return mapping
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
"""BindingResolver protocol and built-in implementations.
2+
3+
This module defines the core extensibility contract for secret resolution.
4+
Each resolver encapsulates one binding source. Compose them into an ordered
5+
chain via :class:`ChainedResolver` — the first resolver that succeeds wins.
6+
7+
Protocol contract::
8+
9+
resolver.resolve(module, instance, target)
10+
11+
- On success: populates ``target`` in-place, returns ``None``
12+
- On failure: raises any exception; the chain tries the next resolver
13+
"""
14+
15+
from __future__ import annotations
16+
17+
from dataclasses import fields, is_dataclass
18+
from typing import Any, Protocol, runtime_checkable
19+
20+
21+
@runtime_checkable
22+
class Resolver(Protocol):
23+
"""Contract for a single binding resolution strategy.
24+
25+
A ``BindingResolver`` reads credentials from one source and populates
26+
``target`` in-place. Implementations raise on failure so that a
27+
:class:`ChainedResolver` can try the next strategy.
28+
29+
Any object implementing ``resolve`` with this signature satisfies the
30+
protocol — no inheritance required.
31+
"""
32+
33+
def resolve(self, module: str, instance: str, target: Any) -> None:
34+
"""Populate ``target`` with credentials for ``module``/``instance``.
35+
36+
Args:
37+
module: Service module name (e.g. ``"destination"``).
38+
instance: Instance identifier (e.g. ``"default"``).
39+
target: Dataclass instance whose ``str`` fields will be set.
40+
41+
Raises:
42+
Any exception on failure; the caller determines how to handle it.
43+
"""
44+
...
45+
46+
47+
class ChainedResolver:
48+
"""Tries each resolver in order; returns on the first success.
49+
50+
Collects failure messages from each resolver and raises a
51+
:class:`RuntimeError` with an aggregated report when all resolvers fail.
52+
53+
Args:
54+
resolvers: Ordered list of :class:`BindingResolver` implementations to try.
55+
base_var_name: Used only for the error guidance message.
56+
"""
57+
58+
def __init__(
59+
self,
60+
resolvers: list[Resolver],
61+
base_var_name: str = "CLOUD_SDK_CFG",
62+
) -> None:
63+
if not resolvers:
64+
raise ValueError("resolvers list must not be empty")
65+
self._resolvers = resolvers
66+
self._base_var_name = base_var_name
67+
68+
def resolve(self, module: str, instance: str, target: Any) -> None:
69+
"""Try each resolver in order; raise on total failure."""
70+
if not is_dataclass(target) or isinstance(target, type):
71+
raise TypeError("target must be a dataclass instance")
72+
for f in fields(target):
73+
if f.type is not str and f.type != "str":
74+
raise TypeError(
75+
f"target field {f.name!r} is not a string (only str fields are supported)"
76+
)
77+
78+
errors: list[str] = []
79+
for resolver in self._resolvers:
80+
try:
81+
resolver.resolve(module, instance, target)
82+
return
83+
except Exception as e:
84+
label = type(resolver).__name__
85+
errors.append(f"{label} failed: {e}")
86+
87+
raise RuntimeError(
88+
f"module={module!r} instance={instance!r} failed to read secrets from all resolvers: "
89+
f"{errors}. "
90+
"Options: mount secrets under the service binding path, set environment variables "
91+
f"like {self._base_var_name}_{module}_{instance}_<KEY> (uppercased), or set VCAP_SERVICES."
92+
)

src/sap_cloud_sdk/core/secret_resolver/constants.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,5 @@
33
"""
44

55
BASE_MOUNT_PATH = "/etc/secrets/appfnd"
6+
BASE_VAR_NAME = "CLOUD_SDK_CFG"
7+
SERVICE_BINDING_ROOT = "SERVICE_BINDING_ROOT"
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
"""Resolver that reads service binding secrets from environment variables."""
2+
3+
import os
4+
from typing import Any
5+
6+
from sap_cloud_sdk.core.secret_resolver._mapping import _get_field_map
7+
from sap_cloud_sdk.core.secret_resolver.constants import BASE_VAR_NAME
8+
9+
10+
class EnvVarResolver:
11+
"""Resolves bindings from environment variables.
12+
13+
Reads variables named ``{base_var_name}_{module}_{instance}_{field_key}``
14+
(uppercased, hyphens in module/instance replaced with underscores).
15+
16+
Args:
17+
base_var_name: Env var name prefix. Defaults to ``"CLOUD_SDK_CFG"``.
18+
"""
19+
20+
def __init__(self, base_var_name: str = BASE_VAR_NAME) -> None:
21+
self._base_var_name = base_var_name
22+
23+
def resolve(self, module: str, instance: str, target: Any) -> None:
24+
"""Load secrets from environment variables."""
25+
normalized_module = module.replace("-", "_")
26+
normalized_instance = instance.replace("-", "_")
27+
_load_from_env(
28+
self._base_var_name, normalized_module, normalized_instance, target
29+
)
30+
31+
32+
def _load_from_env(base_var_name: str, module: str, instance: str, target: Any) -> None:
33+
"""
34+
Load secrets from environment variables with names:
35+
{base_var_name}_{module}_{instance}_{field_key} (uppercased)
36+
instance names have '-' replaced with '_' for env var compatibility.
37+
"""
38+
field_map = _get_field_map(target)
39+
prefix = f"{base_var_name}_{module}_{instance}".upper()
40+
41+
for key, (attr_name, _) in field_map.items():
42+
var_name = f"{prefix}_{key}".upper()
43+
value = os.environ.get(var_name)
44+
if value is None:
45+
# Align with Go: error if env var not found
46+
raise KeyError(f"env var not found: {var_name}")
47+
setattr(target, attr_name, value)

0 commit comments

Comments
 (0)