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

xDS: type url aggregation #9649

Merged
merged 15 commits into from
Mar 18, 2020
Merged

xDS: type url aggregation #9649

merged 15 commits into from
Mar 18, 2020

Conversation

Shikugawa
Copy link
Member

@Shikugawa Shikugawa commented Jan 10, 2020 •
edited
Loading

Signed-off-by: shikugawa [email protected]

Description: This is only refactoring. Aggregated xDS type urls. In previous implementation, loadTypeUrl is scattered on all of xDS subscription classes. That is poor outlook so refactored all of them.

Risk Level: Low
Testing: None
Docs Changes: None
Release Notes: None
[Optional Fixes #Issue] #9468 #9526
[Optional Deprecated:]

@Shikugawa
aggregate xDS resource names
6bd8479 
Signed-off-by: shikugawa <[email protected]>
@Shikugawa
delete unused classes
b548488 
Signed-off-by: shikugawa <[email protected]>
@Shikugawa
delete unused headers
98d126e 
Signed-off-by: shikugawa <[email protected]>
@Shikugawa
Kick CI
6aa3332 
Signed-off-by: shikugawa <[email protected]>
@Shikugawa
resource name -> type url
f4be81e 
Signed-off-by: shikugawa <[email protected]>
@Shikugawa Shikugawa changed the title xDS: resource names aggregation xDS: type url aggregation Jan 12, 2020
@Shikugawa
fix
77e976e 
Signed-off-by: shikugawa <[email protected]>
@Shikugawa
Copy link
Member Author

Shikugawa commented Jan 12, 2020 •
edited
Loading

@htuch I think it also can be eliminate resources.h which can't specify API version. And this will be useful functionality for seamless api version switching that is required such as fallback api version.

@Shikugawa
Kick CI
1ac3c7f 
Signed-off-by: shikugawa <[email protected]>
@stale
Copy link

stale bot commented Jan 21, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 21, 2020
@Shikugawa
Copy link
Member Author

\nostale

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 22, 2020
htuch
htuch reviewed Jan 22, 2020
View reviewed changes
@Shikugawa
add getResourceName()
7a9cd2f 
Signed-off-by: shikugawa <[email protected]>
@stale
Copy link

stale bot commented Feb 5, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 5, 2020
@Shikugawa
fix
217d979 
Signed-off-by: shikugawa <[email protected]>
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 6, 2020
@Shikugawa
fix build error
f1cceb5 
Signed-off-by: shikugawa <[email protected]>
@Shikugawa
Copy link
Member Author

@htuch How is this?

@junr03
Copy link
Member

junr03 commented Feb 7, 2020

@lizan do you mind taking over this?

@junr03 junr03 assigned lizan and unassigned htuch Feb 7, 2020
@lizan lizan added the no stalebot Disables stalebot from closing an issue label Feb 7, 2020
@lizan
Copy link
Member

lizan commented Feb 7, 2020

Yes I will take over this.

@Shikugawa Shikugawa mentioned this pull request Mar 3, 2020
Closed
@lizan lizan removed the no stalebot Disables stalebot from closing an issue label Mar 3, 2020
lizan
lizan suggested changes Mar 3, 2020
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

lizan
lizan reviewed Mar 12, 2020
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

LGTM modulo one net. Thanks!

@lizan lizan mentioned this pull request Mar 12, 2020
Closed
@Shikugawa
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #9649 (comment) was created by @Shikugawa.

see: more, trace.

@Shikugawa
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 9649 in repo envoyproxy/envoy

@Shikugawa
Kick CI
f0444b7 
Signed-off-by: shikugawa <[email protected]>
htuch
htuch reviewed Mar 13, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo naming nit.

@Shikugawa
rename
26e6ba9 
Signed-off-by: shikugawa <[email protected]>
@Shikugawa
Kick CI
9bc83dd 
Signed-off-by: shikugawa <[email protected]>
lizan
lizan approved these changes Mar 18, 2020
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

LGTM, @htuch?

htuch
htuch approved these changes Mar 18, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit ad843aa into envoyproxy:master Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lambdai lambdai lambdai left review comments

@yanavlasov yanavlasov yanavlasov requested changes

@lizan lizan lizan approved these changes

@htuch htuch htuch approved these changes

Assignees

@lizan lizan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Shikugawa @junr03 @lizan @lambdai @yanavlasov @htuch