Skip to content

Adding test proxy AddSanitizers and GetRecordingOptions functions. #24355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

glecaros
Copy link
Contributor

@glecaros glecaros commented Apr 1, 2025

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to module CHANGELOG.md are included.
  • MIT license headers are included in each file.

@Copilot Copilot AI review requested due to automatic review settings April 1, 2025 23:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new test proxy method AddSanitizers along with associated sanitizer types and marshalling functionality to support proxy testing for sanitizers.

  • Added SanitizerDefinition interface and its concrete implementations: HeaderRegexSanitizer, UriRegexSanitizer, and GeneralRegexSanitizer
  • Implemented JSON marshalling for sanitizers and introduced AddSanitizers to send the sanitizers via a POST request
Comments suppressed due to low confidence (1)

sdk/internal/recording/sanitizer.go:131

  • Consider adding tests to verify that AddSanitizers correctly creates the HTTP request and marshals the sanitizers into the expected JSON format.
func AddSanitizers(sanitizers []SanitizerDefinition, options *RecordingOptions) error {

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

sdk/internal

@glecaros glecaros changed the title Adding test proxy AddSanitizers method. Adding test proxy AddSanitizers and GetRecordingOptions functions. Apr 4, 2025
@chlowell
Copy link
Member

chlowell commented Apr 8, 2025

What's the motivation for this? Are you working on an SDK module that adds so many sanitizers that the overhead of adding them individually is meaningful?

@glecaros
Copy link
Contributor Author

glecaros commented Apr 9, 2025

hey @chlowell, I wanted to expose this proxy call to simplify our code a bit; latency was not really a consideration in our package (I am even considering reducing the number of sanitizers we use).
As for the other change, it's made to simplify our testing when we manually run the proxy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants