Skip to content

feat: adapt secret resolver for customization - #217

Merged
betinacosta merged 9 commits into
feat/new-secret-resolverfrom
feat/adapt-secret-resolver-for-custumization
Jul 14, 2026
Merged

feat: adapt secret resolver for customization#217
betinacosta merged 9 commits into
feat/new-secret-resolverfrom
feat/adapt-secret-resolver-for-custumization

Conversation

@betinacosta

@betinacosta betinacosta commented Jul 8, 2026

Copy link
Copy Markdown
Member

Description

This is the first of a series of PRs. I started by doing changes to secret resolver. I tested the changes using the destination module, but I will only modify the modules usage of secret resolver later.

Refactors the secret resolver module to expose a composable, class-based API alongside the existing legacy function. Users can now wire a custom resolver chain once at application startup via configure(SdkConfig(resolver=...)) and all create_client() calls across modules will use it automatically. The built-in MountResolver and EnvVarResolver classes implement the Resolver protocol, and ChainedResolver tries each in order — providing a clear extension point for custom sources (e.g. VCAP_SERVICES, HashiCorp Vault).

The legacy read_from_mount_and_fallback_to_env_var function is preserved for backwards compatibility. Internally it now delegates to the same MountResolver/EnvVarResolver implementations.

Related Issue

Closes #

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Code refactoring
  • Dependency update

How to Test

Run the unit test suite: pytest tests/core/unit/secret_resolver/

Checklist

  • I have read the Contributing Guidelines
  • I have verified that my changes solve the issue
  • I have added/updated automated tests to cover my changes
  • All tests pass locally
  • I have verified that my code follows the Code Guidelines
  • I have updated documentation (if applicable)
  • I have added type hints for all public APIs
  • My code does not contain sensitive information (credentials, tokens, etc.)
  • I have followed Conventional Commits for commit messages

@betinacosta
betinacosta requested a review from a team as a code owner July 8, 2026 18:38
Comment thread src/sap_cloud_sdk/core/secret_resolver/resolver.py
@betinacosta
betinacosta marked this pull request as draft July 8, 2026 18:44
@betinacosta
betinacosta marked this pull request as ready for review July 9, 2026 16:19
@tiagoek

tiagoek commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

SDK Module Review

Check Status Findings
bdd ✅ PASS 0
binding-shape ✅ PASS 0
commits ✅ PASS 0
concurrency ✅ PASS 0
constants ✅ PASS 0
deletion-hygiene ✅ PASS 0
deps-supply ✅ PASS 0
disclosure ✅ PASS 0
docs ✅ PASS 0
errors-logging ⚠️ FLAG 1
hardcode ✅ PASS 0
http-hygiene ✅ PASS 0
license-spdx ✅ PASS 0
pr-size ✅ PASS 0
quality-gate-parity ✅ PASS 0
secrets ✅ PASS 0
telemetry ✅ PASS 0
testing-depth ✅ PASS 0
versioning ❌ BLOCK 1

Findings (2)

2 finding(s): 1 posted as inline comment(s) on the affected lines, 1 not tied to a code line (listed above).


Generated by sdk-review-skill · v1

tiagoek added a commit that referenced this pull request Jul 13, 2026
PR #217 (Betina) triggered BREAKING-01/02 + VER-01 because the breaking
detector treated removal of test methods in TestSecretResolver as a public
API deletion. Test classes (Test*, *Test, *TestCase) are not SDK API — their
methods can be freely added, removed, or renamed without breaking consumers.

Filter: extract_public_class_methods now skips class names matching the test
class convention before extracting the method table.
try:
resolver.resolve(module, instance, target)
return
except Exception as e:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[FLAG] PY-EL-02

except block does not end with raise — swallowing? Add re-raise or comment justifying suppression

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This exception will be raised at the end, with an aggregation of all resolution errors

@betinacosta
betinacosta merged commit ebbda40 into feat/new-secret-resolver Jul 14, 2026
4 checks passed
@betinacosta
betinacosta deleted the feat/adapt-secret-resolver-for-custumization branch July 14, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants