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

chore: use list instead of map for Provider Resources #2658

Merged
merged 4 commits into from
Mar 3, 2024
Merged

chore: use list instead of map for Provider Resources #2658

merged 4 commits into from
Mar 3, 2024

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Feb 20, 2024

  • list adds order stability

@arkodg arkodg requested a review from a team as a code owner February 20, 2024 01:31
@arkodg arkodg requested review from cnvergence and a team and removed request for a team February 20, 2024 01:31
zirain
zirain previously approved these changes Feb 20, 2024
View reviewed changes
zhaohuabing
zhaohuabing previously approved these changes Feb 20, 2024
View reviewed changes
@zirain
Copy link
Contributor

zirain commented Feb 20, 2024

/retest

@cnvergence
Copy link
Member

cnvergence commented Feb 20, 2024 •
edited
Loading

LGTM, tests need to be updated

@zhaohuabing
Copy link
Member

/retest

cnvergence
cnvergence reviewed Feb 20, 2024
View reviewed changes
internal/gatewayapi/resource.go
@@ -20,41 +20,6 @@ import (

type XdsIRMap map[string]*ir.Xds
type InfraIRMap map[string]*ir.Infra
type GatewayClassResources map[string]*Resources
Copy link
Member

@cnvergence cnvergence Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

list will hit a similar issue during Store operation,
it would need a DeepCopy func

panic: watchable.DeepCopy: type is not copiable: *[]*gatewayapi.Resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I had to write my own DeepCopy as well

@Xunzhuo
Copy link
Member

Xunzhuo commented Mar 1, 2024

@arkodg conflicts need to be resolved.

@arkodg arkodg marked this pull request as draft March 2, 2024 02:51
arkodg added 2 commits March 1, 2024 20:01
@arkodg
use list instead of map for Provider Resources
d30da92 
* list adds order stability

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg
fix lint
7a189e2 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg dismissed stale reviews from zhaohuabing and zirain via 7a189e2 March 2, 2024 04:05
@arkodg arkodg marked this pull request as ready for review March 2, 2024 04:08
@codecov Codecov
Copy link

codecov bot commented Mar 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 54.00000% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 63.62%. Comparing base (3e70498) to head (fe19fe9).

Files Patch % Lines
internal/provider/kubernetes/controller.go 57.14% 7 Missing and 2 partials ⚠️
internal/message/types.go 0.00% 8 Missing ⚠️
internal/gatewayapi/resource.go 83.33% 2 Missing and 1 partial ⚠️
internal/gatewayapi/runner/runner.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2658      +/-   ##
==========================================
+ Coverage   63.53%   63.62%   +0.09%     
==========================================
  Files         123      123              
  Lines       20169    20173       +4     
==========================================
+ Hits        12814    12835      +21     
+ Misses       6533     6516      -17     
  Partials      822      822

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arkodg added 2 commits March 1, 2024 20:49
@arkodg
implement Equal to deal with out of order
370ba0c 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg
more tests
fe19fe9 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested review from a team March 2, 2024 04:53
Xunzhuo
Xunzhuo reviewed Mar 2, 2024
View reviewed changes
internal/gatewayapi/resource.go
@@ -20,41 +24,6 @@ import (

type XdsIRMap map[string]*ir.Xds
Copy link
Member

Choose a reason for hiding this comment

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

Should we also use list for xdsIR infraIR instead of map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats an internal structure, iirc each resource is unrelated and the map improves lookup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gateway/internal/ir/xds.go

Line 87 in 3e70498

func (x *Xds) Equal(y *Xds) bool {
helps deal with out of order equality when comparing ir xds

@Xunzhuo
Copy link
Member

Xunzhuo commented Mar 3, 2024

/retest

@arkodg arkodg requested a review from Xunzhuo March 3, 2024 23:44
zirain
zirain approved these changes Mar 3, 2024
View reviewed changes
Copy link
Contributor

@zirain zirain left a comment

Choose a reason for hiding this comment

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

LGTM

@arkodg arkodg merged commit 93d6d21 into envoyproxy:main Mar 3, 2024
16 of 17 checks passed
@arkodg arkodg deleted the list branch March 3, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zirain zirain zirain approved these changes

@zhaohuabing zhaohuabing Awaiting requested review from zhaohuabing

@cnvergence cnvergence Awaiting requested review from cnvergence

@Xunzhuo Xunzhuo Awaiting requested review from Xunzhuo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arkodg @zirain @cnvergence @zhaohuabing @Xunzhuo