Skip to content
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

[Test Proxy] Add "Accept-Language" header to the ignore list - Update RecordMatcher.cs #6152

Closed
wants to merge 1 commit into from

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented May 11, 2023

Ignoring the "Accept-Language" header

@timovv shared the discomfort about the number of times he had to re-record because his locale is en-CA instead of en-US.

And @jeremymeng 's puppeteer upgrade may unveil a pain later on, example below

RecorderError: Unable to find a record for the request POST https://keyvault_name.vault.azure.net/keys/listKeyName-cangettheversionsofakeypaged-/create?api-version=7.4 Header differences: <sec-ch-ua> values differ, request <"HeadlessChrome";v="113", "Chromium";v="113", "Not-A.Brand";v="24">, record <> <Accept-Language> is absent in request, value <en-US> Body differences:

sec-ch-ua is just fixed in #6151

Ignoring the "Accept-Language" header

@timovv shared the discomfort about the number of times he had to re-record because his locale is en-CA instead of en-US.

And @jeremymeng 's puppeteer upgrade may unveil a pain later on, example below
```
RecorderError: Unable to find a record for the request POST https://keyvault_name.vault.azure.net/keys/listKeyName-cangettheversionsofakeypaged-/create?api-version=7.4 Header differences: <sec-ch-ua> values differ, request <"HeadlessChrome";v="113", "Chromium";v="113", "Not-A.Brand";v="24">, record <> <Accept-Language> is absent in request, value <en-US> Body differences:
```

`sec-ch-ua` is just fixed in #6151
@HarshaNalluru HarshaNalluru changed the title [Update RecordMatcher.cs [Test Proxy] Add "Accept-Language" header to the ignore list - Update RecordMatcher.cs May 11, 2023
Copy link
Member

@scbedd scbedd left a comment

Choose a reason for hiding this comment

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

👍

Going to talk to Mike about this one.

@HarshaNalluru
Copy link
Member Author

HarshaNalluru commented May 11, 2023

Upon discussing more with @scbedd..

The issue posted above won't be fixed by adding to the ignore headers...
we'll have to exclude it because ignoring would mean we'll still check for its presence.

And excluding feels wrong as there might be real utility.

cc @jeremymeng

Making it a draft until we conclude

@HarshaNalluru HarshaNalluru requested a review from jeremymeng May 11, 2023 21:51
@HarshaNalluru HarshaNalluru marked this pull request as draft May 11, 2023 21:51
@HarshaNalluru HarshaNalluru requested a review from timovv May 11, 2023 21:52
scbedd

This comment was marked as outdated.

Copy link
Member

@scbedd scbedd left a comment

Choose a reason for hiding this comment

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

Holding off.

@github-actions
Copy link

Hi @HarshaNalluru. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@github-actions github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Jul 21, 2023
@github-actions
Copy link

Hi @HarshaNalluru. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-recent-activity There has been no recent activity on this issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants