Skip to content

refactor package infrastructure/kubernetes#1259

Merged
zirain merged 4 commits intoenvoyproxy:mainfrom
zirain:refactor-component
Apr 20, 2023
Merged

refactor package infrastructure/kubernetes#1259
zirain merged 4 commits intoenvoyproxy:mainfrom
zirain:refactor-component

Conversation

@zirain
Copy link
Copy Markdown
Member

@zirain zirain commented Apr 6, 2023

refactor package to reuse code :

  1. use Applier to create/update/delete Kubernetes resources
  2. use ResourceRender to render resource for Proxy/RateLimit
  3. move common functions to utils package

cc @arkodg

@zirain zirain requested a review from a team as a code owner April 6, 2023 08:04
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 6, 2023

Codecov Report

Merging #1259 (20274eb) into main (5d360cb) will increase coverage by 0.22%.
The diff coverage is 88.96%.

@@            Coverage Diff             @@
##             main    #1259      +/-   ##
==========================================
+ Coverage   61.81%   62.03%   +0.22%     
==========================================
  Files          85       76       -9     
  Lines       10854    10701     -153     
==========================================
- Hits         6709     6638      -71     
+ Misses       3700     3624      -76     
+ Partials      445      439       -6     
Impacted Files Coverage Δ
internal/xds/translator/runner/runner.go 70.00% <0.00%> (ø)
internal/infrastructure/kubernetes/infra.go 68.42% <66.29%> (-31.58%) ⬇️
...frastructure/kubernetes/proxy/resource_provider.go 88.76% <86.40%> (ø)
...tructure/kubernetes/ratelimit/resource_provider.go 97.26% <97.26%> (ø)
internal/cmd/egctl/translate.go 78.67% <100.00%> (ø)
internal/infrastructure/kubernetes/proxy/utils.go 100.00% <100.00%> (ø)
internal/infrastructure/kubernetes/proxy_infra.go 100.00% <100.00%> (+56.75%) ⬆️
.../infrastructure/kubernetes/ratelimit/deployment.go 100.00% <100.00%> (ø)
...ernal/infrastructure/kubernetes/ratelimit/utils.go 100.00% <100.00%> (ø)
...ernal/infrastructure/kubernetes/ratelimit_infra.go 100.00% <100.00%> (+100.00%) ⬆️
... and 1 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zirain zirain force-pushed the refactor-component branch 10 times, most recently from 59ee7b3 to 0255bad Compare April 11, 2023 03:59
@Xunzhuo Xunzhuo requested review from a team, chauhanshubham, kflynn and zhaohuabing and removed request for a team April 11, 2023 07:04
@zirain zirain added this to the 0.5.0-rc1 milestone Apr 12, 2023
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: we can just pass the ns and name to delete a specific resource, or we can use a single function to delete all kinds of resources.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IMO, this PR is too large to review, let's do more refactor later.
Applier absolute can be more generic.

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Apr 17, 2023

@qicz can you also help review this PR since you've done some refactoring in this area in the past, thanks in advance !

@qicz
Copy link
Copy Markdown
Member

qicz commented Apr 17, 2023

@qicz can you also help review this PR since you've done some refactoring in this area in the past, thanks in advance !

No problem. in fact, i have already reviewed a few days ago without.note, i will do this again.

// expectedProxyDeployment returns the expected Deployment based on the provided infra.
func (i *Infra) expectedProxyDeployment(infra *ir.Infra) (*appsv1.Deployment, error) {
type ResourceRender struct {
infra *ir.Infra
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

using ir.ProxyInfra directly?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IMO, it's too wide to use ProxyInfra here for now, change it when necessary in the future.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

address follow up here: #1330 (comment)

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
hejianpeng added 2 commits April 19, 2023 12:21
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
Copy link
Copy Markdown
Member

@qicz qicz left a comment

Choose a reason for hiding this comment

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

LGTM, I will follow up for this refactor in another pr. issue #1330

@zirain zirain merged commit e72598b into envoyproxy:main Apr 20, 2023
@zirain zirain deleted the refactor-component branch April 20, 2023 01:23
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.

4 participants