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

Adapt CLI to use Theia Manager #115

Merged
merged 1 commit into from
Oct 19, 2022
Merged

Conversation

yuntanghsu
Copy link
Contributor

@yuntanghsu yuntanghsu commented Sep 23, 2022

This PR implements the adaption of CLI in order to use Theia Manager, which includes:

  1. Create a theia client with the ca.cert and token.
  2. Use port-forwarder to connect to Theia Manager when running theia out of the cluster.
  3. Add unit test.

Reviewers can focus on the changes under folder pkg/theia/commands.

@yuntanghsu yuntanghsu marked this pull request as draft September 23, 2022 19:43
@yuntanghsu yuntanghsu force-pushed the add_cli_change branch 3 times, most recently from 153764b to 7a20b8c Compare September 27, 2022 20:56
@yuntanghsu yuntanghsu marked this pull request as ready for review September 28, 2022 19:20
@yuntanghsu yuntanghsu added this to the Theia v0.3 Release milestone Sep 28, 2022
@yuntanghsu yuntanghsu force-pushed the add_cli_change branch 3 times, most recently from 0d26e88 to 7c8db42 Compare September 29, 2022 17:27
@codecov-commenter
Copy link

Codecov Report

Merging #115 (7c8db42) into main (8a3826a) will increase coverage by 21.11%.
The diff coverage is 47.92%.

@@             Coverage Diff             @@
##             main     #115       +/-   ##
===========================================
+ Coverage   30.65%   51.76%   +21.11%     
===========================================
  Files          11       11               
  Lines        1442     1161      -281     
===========================================
+ Hits          442      601      +159     
+ Misses        978      482      -496     
- Partials       22       78       +56     
Flag Coverage Δ
unit-tests 51.76% <47.92%> (+21.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/theia/commands/utils.go 37.03% <35.10%> (+6.52%) ⬆️
pkg/theia/commands/policy_recommendation_run.go 61.77% <41.91%> (+35.84%) ⬆️
...g/theia/commands/policy_recommendation_retrieve.go 54.71% <42.85%> (+30.54%) ⬆️
pkg/theia/commands/policy_recommendation_delete.go 53.84% <57.14%> (+45.68%) ⬆️
pkg/theia/commands/policy_recommendation_list.go 70.27% <75.00%> (+68.09%) ⬆️
pkg/theia/commands/policy_recommendation_status.go 64.70% <78.37%> (+39.03%) ⬆️
pkg/theia/commands/policy_recommendation.go 72.72% <100.00%> (-8.53%) ⬇️
... and 2 more

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

pkg/theia/commands/policy_recommendation_delete.go Outdated Show resolved Hide resolved
pkg/theia/commands/utils.go Outdated Show resolved Hide resolved
pkg/theia/commands/utils.go Outdated Show resolved Hide resolved
pkg/theia/commands/utils.go Show resolved Hide resolved
pkg/theia/commands/utils.go Outdated Show resolved Hide resolved
pkg/theia/commands/policy_recommendation_run.go Outdated Show resolved Hide resolved
pkg/theia/commands/policy_recommendation_run.go Outdated Show resolved Hide resolved
pkg/theia/commands/policy_recommendation_status.go Outdated Show resolved Hide resolved
pkg/theia/commands/utils_test.go Show resolved Hide resolved
@yuntanghsu yuntanghsu force-pushed the add_cli_change branch 4 times, most recently from 728b431 to b7ac23b Compare October 3, 2022 22:48
docs/networkpolicy-recommendation.md Outdated Show resolved Hide resolved
pkg/theia/commands/utils.go Outdated Show resolved Hide resolved
pkg/theia/commands/policy_recommendation_delete.go Outdated Show resolved Hide resolved
pkg/theia/commands/policy_recommendation_delete.go Outdated Show resolved Hide resolved
pkg/theia/commands/utils.go Outdated Show resolved Hide resolved
Host: host,
BearerToken: token,
TLSClientConfig: restclient.TLSClientConfig{
Insecure: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to expose Insecure as cli arg to be fail-safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not? If we need Insecure=true, does it mean we have a wrong ca-crt? Or maybe we have some issues in Theia-manager? For both cases, do we still want to connect to Theia-manager?

@yuntanghsu yuntanghsu force-pushed the add_cli_change branch 6 times, most recently from e4db1fd to 0b4f6a8 Compare October 7, 2022 22:22
@yuntanghsu
Copy link
Contributor Author

/theia-test-e2e

@yuntanghsu yuntanghsu requested a review from dreamtalen October 10, 2022 23:49
@yuntanghsu
Copy link
Contributor Author

/theia-test-e2e

@yuntanghsu yuntanghsu force-pushed the add_cli_change branch 4 times, most recently from 8536ed3 to 43b1019 Compare October 18, 2022 16:45
@yuntanghsu
Copy link
Contributor Author

/theia-test-e2e

Copy link

@dreamtalen dreamtalen left a comment

Choose a reason for hiding this comment

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

Only few nits, LGTM

pkg/theia/commands/policy_recommendation_run.go Outdated Show resolved Hide resolved
pkg/theia/commands/utils.go Outdated Show resolved Hide resolved
1.Create a theia client with the ca.cert and token.
2.Use port-forwarder to connect to Theia Manager when running theia out of the cluster.
3.Add unit test.

Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
@yuntanghsu
Copy link
Contributor Author

/theia-test-e2e

Copy link
Contributor

@yanjunz97 yanjunz97 left a comment

Choose a reason for hiding this comment

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

LGTM

@yuntanghsu yuntanghsu merged commit e7bd73a into antrea-io:main Oct 19, 2022
@yuntanghsu yuntanghsu deleted the add_cli_change branch October 19, 2022 00:07
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.

5 participants