-
Notifications
You must be signed in to change notification settings - Fork 4
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
restructuring inspector-driver integration #10
Conversation
ce69688
to
978af3b
Compare
Oh yeah I was able to run the simple dashboard branch on Imani's windows 11 cmdprompt and it worked @MeNsaaH |
…ido into feature/inspector-restructure
driver/driver.go
Outdated
Name string | ||
Extra string | ||
} | ||
|
||
type fields struct { | ||
// Supported inspector representations for specific driver | ||
Supported []string |
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.
We should be able to take this out, do we still need it?
driver/driver.go
Outdated
Name string | ||
Extra string | ||
} | ||
|
||
type fields struct { | ||
// Supported inspector representations for specific driver | ||
Supported []string | ||
// Selected inspector representations | ||
Selected []string |
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.
And this as well
driver/driver.go
Outdated
Name string | ||
Extra string | ||
} | ||
|
||
type fields struct { |
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.
you think we can make this something else or give it a more concrete name? like driverBase
driver/ssh.go
Outdated
@@ -27,7 +27,8 @@ type SSH struct { | |||
// Check known hosts (only disable for tests | |||
CheckKnownHosts bool | |||
// set environmental vars for server e.g []string{"DEBUG=1", "FAKE=echo"} | |||
Vars []string | |||
Vars []string |
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.
Should be EnvVars
. Vars
is a bit ambigous.
driver/ssh.go
Outdated
@@ -72,7 +77,7 @@ func (d *SSH) ReadFile(path string) (string, error) { | |||
} | |||
|
|||
func (d *SSH) RunCommand(command string) (string, error) { | |||
// FIXME: Do we retain client across all command runs? | |||
// FIXME: provide refresh interface to somehow refresh d.Client if d.SessionClient somehow gets dropped |
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.
I think we sould retain the client across all commands by the driver. That way reduce the number of connections we have to make. We should probably not call defer client.Close()
and close all the connections when we're done
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.
When exactly do we close the connections
if d.Info == nil { | ||
uname, err := d.RunCommand(`uname`) | ||
// try windows command | ||
if err != nil { |
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.
Probably need to check the error could be other errors. Like ssh connection errors.
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.
And then what do we do at that point
@@ -21,7 +21,7 @@ const ( | |||
GET Request = "GET" | |||
) | |||
|
|||
// Web : Driver for handling ssh executions | |||
// Web : Driver for handling web executions |
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.
Why exactly do we have web drivers again 🙈 ?
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.
Lol... to help monitor web applications (we haven't fleshed out the inspectors for those but it is very valid)
No description provided.