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

egctl: implement config envoy-proxy all command #1060

Merged
merged 11 commits into from
Feb 28, 2023
Merged

Conversation

zirain
Copy link
Contributor

@zirain zirain commented Feb 18, 2023

xref: #915

@zirain zirain marked this pull request as ready for review February 18, 2023 08:13
@zirain zirain requested a review from a team as a code owner February 18, 2023 08:13
@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2023

Codecov Report

Merging #1060 (20d8811) into main (2c57f3d) will decrease coverage by 0.61%.
The diff coverage is 10.52%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1060      +/-   ##
==========================================
- Coverage   64.33%   63.73%   -0.61%     
==========================================
  Files          70       71       +1     
  Lines        9012     9248     +236     
==========================================
+ Hits         5798     5894      +96     
- Misses       2820     2948     +128     
- Partials      394      406      +12     
Impacted Files Coverage Δ
internal/cmd/egctl/config.go 10.52% <10.52%> (ø)
internal/cmd/egctl/translate.go 67.57% <0.00%> (-3.26%) ⬇️
internal/gatewayapi/helpers.go 75.00% <0.00%> (-1.78%) ⬇️
internal/provider/kubernetes/controller.go 50.33% <0.00%> (-0.46%) ⬇️
internal/cmd/egctl/version.go 0.00% <0.00%> (ø)
internal/gatewayapi/route.go 86.33% <0.00%> (+1.20%) ⬆️
internal/xds/translator/authentication.go 64.31% <0.00%> (+2.31%) ⬆️

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

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

arkodg commented Feb 20, 2023

shouldnt the title be egctl config envoy-proxy all <instance_name>

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain zirain requested a review from arkodg February 21, 2023 03:23
@arkodg arkodg changed the title egctl: implement config all command egctl: implement config envoy-proxy all command Feb 21, 2023
zirain and others added 2 commits February 22, 2023 08:50
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
Signed-off-by: zirain <hejianpeng2@huawei.com>
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
Signed-off-by: zirain <hejianpeng2@huawei.com>
@zirain zirain requested a review from arkodg February 22, 2023 02:04
@arkodg
Copy link
Contributor

arkodg commented Feb 22, 2023

thanks @zirain this looks like a great start, hoping you can add some test cases since the current diff coverage for this PR is 0%

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@@ -12,3 +12,4 @@
go.mod
go.sum
bin
testdata
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't right, we were linting testdata earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me ignore special file, configdump file will not pass codespell check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

I pulled in your branch and deleted the tracer that was causing the issue

diff --git a/internal/cmd/egctl/testdata/config/out.json b/internal/cmd/egctl/testdata/config/out.json
index d7bc3d3..1bd6f16 100644
--- a/internal/cmd/egctl/testdata/config/out.json
+++ b/internal/cmd/egctl/testdata/config/out.json
@@ -1316,10 +1316,6 @@
                                 "envoy.extensions.filters.network.thrift_proxy.router.v3.Router"
                             ]
                         },
-                        {
-                            "name": "envoy.dynamic.ot",
-                            "category": "envoy.tracers"
-                        },

this fixed codespell

Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
Xunzhuo
Xunzhuo previously approved these changes Feb 25, 2023
Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

Thanks @zirain

@zirain zirain mentioned this pull request Feb 25, 2023
10 tasks
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
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 ead74ab into envoyproxy:main Feb 28, 2023
@zirain zirain deleted the egctl-2 branch February 28, 2023 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants