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

Add ClusterIP to the services in the egctl test data #1758

Merged
merged 4 commits into from
Aug 5, 2023

Conversation

Ronnie-personal
Copy link
Contributor

@Ronnie-personal Ronnie-personal commented Aug 3, 2023

What type of PR is this? fix: fix egctl test data, also add endpoints ValidateAll().

What this PR does / why we need it: xds endpoint configuration requires address under socketAddress field. We need to provide ClusterIP in the input data file.

Which issue(s) this PR fixes: #1742

Fixes #

Co-authored-by: tony-2023 <138949958+tony-2023@users.noreply.github.com>
Signed-off-by: Ronnie Personal <76408835+Ronnie-personal@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #1758 (3715b44) into main (f8cf3e7) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1758      +/-   ##
==========================================
+ Coverage   65.02%   65.13%   +0.10%     
==========================================
  Files          84       84              
  Lines       12144    12151       +7     
==========================================
+ Hits         7897     7914      +17     
+ Misses       3740     3733       -7     
+ Partials      507      504       -3     
Files Changed Coverage Δ
internal/xds/types/resourceversiontable.go 86.23% <100.00%> (+1.92%) ⬆️

... and 3 files with indirect coverage changes

@Ronnie-personal Ronnie-personal marked this pull request as ready for review August 3, 2023 02:39
@Ronnie-personal Ronnie-personal requested a review from a team as a code owner August 3, 2023 02:39
@arkodg
Copy link
Contributor

arkodg commented Aug 3, 2023

thanks @Ronnie-personal ! does this allow you to add the ValidateAll() method back for Endpoints ?

@Ronnie-personal
Copy link
Contributor Author

thanks @Ronnie-personal ! does this allow you to add the ValidateAll() method back for Endpoints ?

Yes, we will be able to add endpoints ValidateAll(), after this change.
I tested locally, egctl tests passed with ValidateAll() in place.

@arkodg
Copy link
Contributor

arkodg commented Aug 3, 2023

Can you add ValidateAll into this PR as well ?

@Ronnie-personal
Copy link
Contributor Author

Can you add ValidateAll into this PR as well ?

Sure, will do.

@Ronnie-personal Ronnie-personal marked this pull request as draft August 4, 2023 00:02
Signed-off-by: Ronnie Personal <76408835+Ronnie-personal@users.noreply.github.com>
Signed-off-by: Ronnie Personal <76408835+Ronnie-personal@users.noreply.github.com>
@Ronnie-personal Ronnie-personal marked this pull request as ready for review August 4, 2023 02:32
Copy link
Contributor

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

@zirain zirain merged commit 3c348d9 into envoyproxy:main Aug 5, 2023
18 checks passed
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.

None yet

3 participants