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.

Sign up for GitHub

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

Jump to bottom

feat(request_id): set request id when it's set but empty #38474

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

feat(request_id): set request id when it's set but empty #38474

wants to merge 6 commits into from
+51 −1

Conversation

quantumsheep
Copy link
Contributor

@quantumsheep quantumsheep commented Feb 17, 2025 •
edited
Loading

Commit Message: feat(request_id): set request id when it's set but empty
Risk Level: Low
Testing: Yes
Docs Changes: N/A
Release Notes:

  • generate_request_id will generate a request id on non-present and empty x-request-id header.

Fixes #38445

@wbpcode wbpcode self-assigned this Feb 17, 2025
@wbpcode
Copy link
Member

wbpcode commented Feb 17, 2025

Please check the CI. Thanks.

@wbpcode
Copy link
Member

wbpcode commented Feb 17, 2025

/retest

@quantumsheep
Copy link
Contributor Author

@wbpcode I'm not sure about the current error

Error: ERROR: Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE. Either remove the 'shard_count' attribute, use a test runner that supports sharding or temporarily disable this check via --noincompatible_check_sharding_support.

@wbpcode
Copy link
Member

wbpcode commented Feb 18, 2025

@wbpcode I'm not sure about the current error

Error: ERROR: Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE. Either remove the 'shard_count' attribute, use a test runner that supports sharding or temporarily disable this check via --noincompatible_check_sharding_support.

could you merge main and kick the ci again?

@quantumsheep quantumsheep force-pushed the feat/request-id-fill-empty branch 2 times, most recently from 4cffd76 to c9ad7f6 Compare February 19, 2025 09:04
@quantumsheep
Copy link
Contributor Author

could you merge main and kick the ci again?

The issue persists :( Do you know why it happens?

wbpcode
wbpcode previously approved these changes Feb 20, 2025
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@wbpcode
Copy link
Member

wbpcode commented Feb 20, 2025

cc @quantumsheep could you also add a change log under the minor behavior change scope? Then I can merge this PR.

@wbpcode
Copy link
Member

wbpcode commented Feb 20, 2025

/wait

@wbpcode wbpcode dismissed their stale review February 20, 2025 02:46

need change log

@quantumsheep quantumsheep force-pushed the feat/request-id-fill-empty branch from ad63b3c to db41e87 Compare February 20, 2025 09:25
@quantumsheep
Copy link
Contributor Author

quantumsheep commented Feb 20, 2025 •
edited
Loading

Changelog updated. I'm not sure about the area though. Is it fine?

@quantumsheep
feat(request_id): set request id when it's set but empty
17bf08d 
Signed-off-by: Nathanael DEMACON <[email protected]>
@quantumsheep
Better tests
8554086 
Signed-off-by: Nathanael DEMACON <[email protected]>
@quantumsheep
revert test
fb7abfe 
Signed-off-by: Nathanael DEMACON <[email protected]>
@quantumsheep
format
31e0a5a 
Signed-off-by: Nathanael DEMACON <[email protected]>
@quantumsheep
add changelog
87b9f58 
Signed-off-by: Nathanael DEMACON <[email protected]>
@quantumsheep quantumsheep force-pushed the feat/request-id-fill-empty branch from db41e87 to 87b9f58 Compare February 20, 2025 13:15
@wbpcode
Copy link
Member

wbpcode commented Feb 20, 2025

Changelog updated. I'm not sure about the area though. Is it fine?

I think http would be better.

@quantumsheep
http
be5adc9 
Signed-off-by: Nathanael DEMACON <[email protected]>
@quantumsheep
Copy link
Contributor Author

Done 👌

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

@wbpcode wbpcode wbpcode left review comments

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

@alyssawilk alyssawilk Awaiting requested review from alyssawilk alyssawilk is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@wbpcode wbpcode

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

generate_request_id does not generate x-request-id if it is present but empty
2 participants
@quantumsheep @wbpcode