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

Identity on off cmdline #323

Merged
merged 31 commits into from
Apr 8, 2021
Merged

Identity on off cmdline #323

merged 31 commits into from
Apr 8, 2021

Conversation

mary-dcouto
Copy link
Contributor

This PR also contains loglevel and feedback commands in the cmdline option

@mary-dcouto mary-dcouto requested a review from dovholuknf March 11, 2021 10:42
@@ -609,7 +613,7 @@ func toggleIdentity(out *json.Encoder, fingerprint string, onOff bool) {
msg := fmt.Sprintf("identity with fingerprint %s not found", fingerprint)
log.Warn(msg)
respond(out, dto.Response{
Code: SUCCESS,
Code: IDENTITY_NOT_FOUND,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clint, I modified this code. If the fingerprint value is not matching, the Identity not found code will be set rather than success. Not sure whether it will affect any other code

Copy link
Member

Choose a reason for hiding this comment

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

this makes me think that there should be something in the UI that should be updated?

@@ -25,630 +25,755 @@

Copy link
Member

Choose a reason for hiding this comment

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

can you revert this file pls? i don't expect you made any changes here?

Copy link
Member

Choose a reason for hiding this comment

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

or maybe it just needs to be merged? i didn't expect to see any ".cs" changes on this pr?

@dovholuknf dovholuknf changed the base branch from main to release-next April 6, 2021 20:20
@@ -484,6 +484,7 @@ public partial class MainWindow : Window {
serviceClient.OnServiceEvent += ServiceClient_OnServiceEvent;
serviceClient.OnTunnelStatusEvent += ServiceClient_OnTunnelStatusEvent;
serviceClient.OnMfaEvent += ServiceClient_OnMfaEvent;
serviceClient.OnLogLevelEvent += ServiceClient_OnLogLevelEvent;
Copy link
Member

Choose a reason for hiding this comment

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

looks like you need to use tabs in .cs files to be consistent

return
} else {
if responseMsg.Error != "" {
log.Info(responseMsg.Error)
Copy link
Member

Choose a reason for hiding this comment

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

probably log.Error?

if tunnelStatus.Status != nil {
responseMsg := fn(args, tunnelStatus.Status, flags)
if responseMsg.Code == service.SUCCESS {
log.Info("\n" + responseMsg.Payload.(string) + "\n" + responseMsg.Message)
Copy link
Member

Choose a reason for hiding this comment

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

i probably would have used to statements here. also starting the log statement off with a new line was probably done on purpose but it just seems strange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the new line because I want the result in new line. Otherwise, it will get appended to the date time stamp of the log. could not remove the date time stamp from the log statements yet

Copy link
Member

Choose a reason for hiding this comment

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

i'd say update the formatter then or don't use log.Info and just use fmt.Println for output like this...

}
}
} else {
log.Errorf("Ziti tunnel retuned nil status")
Copy link
Member

Choose a reason for hiding this comment

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

is it helpful to log the status too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

status is nil. This happens when the pipe is not connected or the ziti-tunnel instance is not running

return
} else {
if responseMsg.Error != "" {
log.Info(responseMsg.Error)
Copy link
Member

Choose a reason for hiding this comment

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

log.Error?

}
}
} else {
log.Errorf("Ziti tunnel retuned nil response")
Copy link
Member

Choose a reason for hiding this comment

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

log the response? is it helpful to see?

if err != nil {
log.Errorf("Connection to ipc pipe is not established, %v", err)
log.Errorf("Ziti Desktop Edge app may not be running")
return false
Copy link
Member

Choose a reason for hiding this comment

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

probably could log.fatal ?

@@ -500,6 +500,10 @@ func serveIpc(conn net.Conn) {

//save the state
rts.SaveState()
case "NotifyLogLevelUIAndUpdateService":
sendLogLevelAndNotify(enc, cmd.Payload["Level"].(string))
Copy link
Member

Choose a reason for hiding this comment

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

you don't need NotifyLogLevelUIAndUpdateService nor NotifyIdentityUI. Both of those should happen already. setLogLevel should broadcast the log level event change and NotifyIdentityUI should be changed to send an identity event from toggleIdentity. let's discuss if that's not clear

@dovholuknf dovholuknf merged commit 66d277e into release-next Apr 8, 2021
@dovholuknf dovholuknf deleted the identity-onOff-cmdline branch April 8, 2021 10:56
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.

3 participants