Skip to content

fix(agentgateway): address Nicole review feedback on PR #220#234

Merged
tiagoek merged 1 commit into
feat/transparent-tls-smahrfrom
fix/pr-220-nicole-review-cleanup
Jul 17, 2026
Merged

fix(agentgateway): address Nicole review feedback on PR #220#234
tiagoek merged 1 commit into
feat/transparent-tls-smahrfrom
fix/pr-220-nicole-review-cleanup

Conversation

@tiagoek

@tiagoek tiagoek commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

Addresses Nicole's review comments on PR #220 (Transparent TLS + ServiceBinding Envelope Support). Stacked on feat/transparent-tls-smahr (same pattern as #230).

Changes:

  • Delete src/sap_cloud_sdk/core/_telemetry_compat.py and restore direct sap_cloud_sdk.core.telemetry imports in agentgateway/destination modules (telemetry remains a required dependency; use CLOUD_SDK_OTEL_DISABLED=true to disable locally)
  • Remove unrelated test-agent/ entry from .gitignore
  • Revert unnecessary import reorder in tests/agentgateway/unit/test_lob.py
  • Keep package version at 0.33.18 (unchanged)

Related Issue

No GitHub/Jira issue yet (follow-up cleanup for #220).

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

Describe how reviewers can test your changes:

  1. uv sync --group dev
  2. uv run pytest -m "not integration" tests/agentgateway/unit/ tests/destination/unit/ -q
  3. Expected result: all unit tests pass; no imports of _telemetry_compat

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

None.

Additional Notes

@tiagoek
tiagoek force-pushed the fix/pr-220-nicole-review-cleanup branch from 5385b0a to 69bcf8f Compare July 17, 2026 16:20
@tiagoek
tiagoek marked this pull request as ready for review July 17, 2026 16:40
@tiagoek
tiagoek requested a review from a team as a code owner July 17, 2026 16:40
@tiagoek
tiagoek merged commit c4b1ba2 into feat/transparent-tls-smahr Jul 17, 2026
2 of 4 checks passed
@tiagoek
tiagoek deleted the fix/pr-220-nicole-review-cleanup branch July 17, 2026 16:40
tiagoek added a commit that referenced this pull request Jul 17, 2026
Include sdk-review quality findings and Nicole review cleanup that were
previously landed on the wrong SAP branch (feat/transparent-tls-smahr).
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