Skip to content

feat:add new secret resolver to modules#231

Open
betinacosta wants to merge 21 commits into
feat/new-secret-resolverfrom
feat/add-new-secret-resolver-to-modules
Open

feat:add new secret resolver to modules#231
betinacosta wants to merge 21 commits into
feat/new-secret-resolverfrom
feat/add-new-secret-resolver-to-modules

Conversation

@betinacosta

Copy link
Copy Markdown
Member

Description

This PR implements the secret resolver into the sdk modules.

Type of Change

Please check the relevant option:

  • 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

from sap_cloud_sdk import (
    adms,
    agent_memory,
    agentgateway,
    destination,
    dms,
    objectstore,
    print as print_service
)

from sap_cloud_sdk.core import (
    auditlog_ng,
    data_anonymization
)


def main() -> None:
    # Destination client
    destination_client = destination.create_client()
    print(f"Destination: {destination_client}")

    # ADMS client
    adms_client = adms.create_client()
    print(f"ADMS: {adms_client}")

    # Agent Gateway
    agent_gateway_client = agentgateway.create_client()
    print(f"Agent Gateway: {agent_gateway_client}")

    # Agent Memory
    agent_memory_client = agent_memory.create_client()
    print(f"Agent Memory: {agent_memory_client}")

    # DMS
    dms_client = dms.create_client()
    print(f"DMS: {dms_client}")

    # Object Store
    object_store_client = objectstore.create_client("default")
    print(f"Object Store: {object_store_client}")

    # Print Service
    print_client = print_service.create_client()
    print(f"Print: {print_client}")

    # Auditlog V3
    auditlog_client = auditlog_ng.create_client()
    print(f"Auditlog NG: {auditlog_client}")

    # Data Anonymization
    data_client = data_anonymization.create_client()
    print(f"Data Anonymization: {data_client}")

    

if __name__ == "__main__":
    main()

Checklist

Before submitting your PR, please review and check the following:

  • 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

Breaking Changes

If this PR introduces breaking changes, please describe:

  • What breaks
  • Migration path for users
  • Alternative approaches considered

Additional Notes

Add any additional context, screenshots, or information that would help reviewers.

@betinacosta
betinacosta requested a review from a team as a code owner July 15, 2026 14:20
@betinacosta
betinacosta marked this pull request as draft July 15, 2026 20:11
@betinacosta
betinacosta marked this pull request as ready for review July 16, 2026 12:48

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.

This method shouldn't exist anymore

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.

Maybe instead of module, could it be service_name and instance_name?

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.

Could be moved to resolve.

I'm just thinking this is not an EnvVarResolver generic, because it expects module and instance. Should we maybe have an SdkEnvVarResolver and an EnvVarResolver just with the variable name?

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.

Same points here.

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.

I believe this configuration could be global or per module.

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.

Aren't we missing an BindingRootResolver indeed?

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.

Suggested change
def get() -> Optional[SdkConfig]:

No need to duplicate sdk_config in the name since you are already have a class.

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.

Maybe we will need to have a generic list of args or an interface each resolver implement for ResolverParameters.



def _load_config_from_env(instance: str = "default") -> DataAnonymizationConfig:
def _load_secrets(instance: str = "default") -> DataAnonymizationConfig:

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.

I believe this encapsulation is not needed. We could also have an central enum for module names if we need it, instead of magic strings. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants