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

bootstrap: add String method to ServerConfigs type #7537

Merged
merged 3 commits into from
Aug 20, 2024
Merged

bootstrap: add String method to ServerConfigs type #7537

merged 3 commits into from
Aug 20, 2024

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Aug 19, 2024

RELEASE NOTES: none

@easwars easwars requested a review from arvindbr8 August 19, 2024 20:40
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Aug 19, 2024
@easwars easwars added this to the 1.67 Release milestone Aug 19, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 30.76923% with 9 lines in your changes missing coverage. Please review.

Project coverage is 81.86%. Comparing base (86135c3) to head (dbf5688).
Report is 9 commits behind head on master.

Files Patch % Lines
internal/xds/bootstrap/bootstrap.go 18.18% 8 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7537      +/-   ##
==========================================
+ Coverage   81.82%   81.86%   +0.04%     
==========================================
  Files         360      360              
  Lines       27533    27508      -25     
==========================================
- Hits        22528    22520       -8     
+ Misses       3804     3793      -11     
+ Partials     1201     1195       -6
Files Coverage Δ
internal/testutils/xds/e2e/bootstrap.go 64.00% <100.00%> (ø)
internal/xds/bootstrap/bootstrap.go 70.47% <18.18%> (-1.88%) ⬇️

... and 28 files with indirect coverage changes

arvindbr8
arvindbr8 approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, unless you think the stringer method might benefit from unit tests.

Also on a side note: Should we modify NewContentsForTesting to Unmarshal to ServerConfigs directly?

@arvindbr8 arvindbr8 assigned easwars and unassigned arvindbr8 Aug 19, 2024
@easwars
Copy link
Contributor Author

easwars commented Aug 20, 2024

Also on a side note: Should we modify NewContentsForTesting to Unmarshal to ServerConfigs directly?

That's a good idea. Did that.

@easwars
Copy link
Contributor Author

easwars commented Aug 20, 2024

Overall LGTM, unless you think the stringer method might benefit from unit tests.

Thought about it. It will mostly be used only for logging and/or debugging. So. I'm not too worried about not having unit tests for that. In fact, we don't want anything to rely on this output being stable.

@easwars easwars merged commit 7e12068 into grpc:master Aug 20, 2024
13 checks passed
@easwars easwars deleted the xdsclient_connectivity_state branch August 20, 2024 21:08
@tbg tbg mentioned this pull request Nov 27, 2024
Draft
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arvindbr8 arvindbr8 arvindbr8 approved these changes

Assignees

@easwars easwars

Labels
Type: Internal Cleanup Refactors, etc
Projects
None yet
Milestone
1.67 Release
Development

Successfully merging this pull request may close these issues.
2 participants
@easwars @arvindbr8