-
Notifications
You must be signed in to change notification settings - Fork 26
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
Conversation
153764b
to
7a20b8c
Compare
7a20b8c
to
930c0c9
Compare
0d26e88
to
7c8db42
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
728b431
to
b7ac23b
Compare
b7ac23b
to
b2b9648
Compare
Host: host, | ||
BearerToken: token, | ||
TLSClientConfig: restclient.TLSClientConfig{ | ||
Insecure: false, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
e4db1fd
to
0b4f6a8
Compare
/theia-test-e2e |
0b4f6a8
to
a7915b8
Compare
/theia-test-e2e |
8536ed3
to
43b1019
Compare
/theia-test-e2e |
There was a problem hiding this 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
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>
43b1019
to
3a114eb
Compare
/theia-test-e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR implements the adaption of CLI in order to use Theia Manager, which includes:
Reviewers can focus on the changes under folder pkg/theia/commands.