Skip to content

Only record headers if needed #450

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 7 commits into
base: main
Choose a base branch
from
Open

Only record headers if needed #450

wants to merge 7 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented May 1, 2025

Proof of concept. Fixes #417.

@sckott started on this so you'd have something to give concrete feedback on. I do like that it eliminates the chances of leaking keys for the majority of APIs.

Proof of concept. Fixes #417.
@sckott sckott added this to the v2.0 milestone May 2, 2025
@sckott
Copy link
Collaborator

sckott commented May 5, 2025

I think I resolved the conflict correctly, change if not.

@hadley
Copy link
Member Author

hadley commented May 5, 2025

Yeah, that looks reasonable to me.

Copy link
Collaborator

@sckott sckott left a comment

Choose a reason for hiding this comment

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

We're dropping the headers slot from the interaction written to disk, but we're leaving an empty request body slot there still as body: {}. Maybe we should be consistent about that? Or leave body as is?

We should document this request header behavior for users. Probably in the param def for match_requests_on in use_cassette(), and maybe in the getting started vignette

@sckott
Copy link
Collaborator

sckott commented May 5, 2025

I do like that it eliminates the chances of leaking keys for the majority of APIs.

me too

@hadley
Copy link
Member Author

hadley commented May 5, 2025

Yeah, let me apply the same treatment to body, and add some extra docs.

@hadley
Copy link
Member Author

hadley commented May 5, 2025

Do you think we should also drop the body if body isn't included in the request matchers?

...

I went ahead and did it since it seems the most consistnent.

@hadley
Copy link
Member Author

hadley commented May 5, 2025

I also need to update the secrets vignette.

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.

Only store request headers if used?
2 participants