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

Support retrieving the initial destination CID from GetParam #3755

Merged
merged 11 commits into from
Jul 15, 2023
Merged

Support retrieving the initial destination CID from GetParam #3755

merged 11 commits into from
Jul 15, 2023

Conversation

ProjectsByJackHe
Copy link
Contributor

@ProjectsByJackHe ProjectsByJackHe commented Jul 13, 2023 •
edited by nibanks
Loading

Description

This PR fixes #3613 that requests a new feature which allows clients and servers to get the original destination CID of the client that initialized the connection.

Testing

A new test was added that checks the various fail conditions and edge cases.

Documentation

Yes, a new GetParameter was added.

@ProjectsByJackHe ProjectsByJackHe requested a review from a team as a code owner July 13, 2023 00:08
csujedihy
csujedihy reviewed Jul 13, 2023
View reviewed changes
src/core/connection.c Outdated Show resolved Hide resolved
@csujedihy
Copy link
Contributor

The title should reflect the functionality of the new parameter. I'd write "Support retrieving the initial destination CID from GetParam".

csujedihy
csujedihy reviewed Jul 13, 2023
View reviewed changes
src/test/lib/ApiTest.cpp Outdated Show resolved Hide resolved
csujedihy
csujedihy reviewed Jul 13, 2023
View reviewed changes
src/test/lib/ApiTest.cpp Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2023 •
edited
Loading

Codecov Report

Merging #3755 (e21c9d8) into main (d8414ff) will decrease coverage by 3.49%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main    #3755      +/-   ##
==========================================
- Coverage   78.82%   75.34%   -3.49%     
==========================================
  Files          56       56              
  Lines       15535    15549      +14     
==========================================
- Hits        12246    11715     -531     
- Misses       3289     3834     +545
Impacted Files Coverage Δ
src/core/connection.c 71.79% <85.71%> (-5.73%) ⬇️

... and 20 files with indirect coverage changes

csujedihy
csujedihy reviewed Jul 13, 2023
View reviewed changes
src/core/connection.c Outdated Show resolved Hide resolved
nibanks
nibanks reviewed Jul 13, 2023
View reviewed changes
src/core/connection.c Outdated Show resolved Hide resolved
@ProjectsByJackHe ProjectsByJackHe changed the title Add a new GetParameter and unit test to ensure behavior Support retrieving the initial destination CID from GetParam Jul 13, 2023
nibanks
nibanks reviewed Jul 13, 2023
View reviewed changes
src/test/lib/ApiTest.cpp Outdated Show resolved Hide resolved
ami-GS
ami-GS reviewed Jul 13, 2023
View reviewed changes
src/test/lib/ApiTest.cpp Outdated Show resolved Hide resolved
nibanks
nibanks reviewed Jul 13, 2023
View reviewed changes
src/core/connection.c Outdated Show resolved Hide resolved
nibanks
nibanks reviewed Jul 13, 2023
View reviewed changes
src/core/connection.c Outdated Show resolved Hide resolved
nibanks
nibanks reviewed Jul 13, 2023
View reviewed changes
docs/Settings.md Outdated Show resolved Hide resolved
nibanks
nibanks reviewed Jul 13, 2023
View reviewed changes
docs/Settings.md Outdated Show resolved Hide resolved
nibanks
nibanks reviewed Jul 14, 2023
View reviewed changes
src/core/connection.c Show resolved Hide resolved
src/test/lib/ApiTest.cpp Show resolved Hide resolved
nibanks
nibanks previously approved these changes Jul 14, 2023
View reviewed changes
Copy link
Member

@nibanks nibanks 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!

@nibanks
Copy link
Member

nibanks commented Jul 14, 2023

Looks like the CheckDotnet job failed because the .NET abstraction for our API wasn't updated to stay in sync with the change to the API. Please run .\scripts\generate-dotnet.ps1 and commit the changes. Thanks!

@nibanks nibanks added Area: API Area: Core Related to the shared, core protocol logic labels Jul 14, 2023
@ProjectsByJackHe
Copy link
Contributor Author

Looks like the CheckDotnet job failed because the .NET abstraction for our API wasn't updated to stay in sync with the change to the API. Please run .\scripts\generate-dotnet.ps1 and commit the changes. Thanks!

Done!

@ProjectsByJackHe ProjectsByJackHe self-assigned this Jul 14, 2023
@nibanks nibanks merged commit 21ea497 into microsoft:main Jul 15, 2023
427 of 776 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ami-GS ami-GS ami-GS left review comments

@csujedihy csujedihy csujedihy left review comments

@nibanks nibanks nibanks approved these changes

Assignees

@ProjectsByJackHe ProjectsByJackHe

Labels
Area: API Area: Core Related to the shared, core protocol logic
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose the initial client CID
5 participants
@ProjectsByJackHe @csujedihy @codecov-commenter @nibanks @ami-GS