-
Notifications
You must be signed in to change notification settings - Fork 6
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
vbox driver #405
vbox driver #405
Conversation
} | ||
|
||
func capabilitiesScanner() ([]string, error) { | ||
l := make([]string, 0) |
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.
l := make([]string, 0)
if _, err := exec.LookPath("VBoxManage"); err == nil {
l = append(l, drvID.Cap())
return []string{} | ||
} | ||
//cf := t.configFile() | ||
cf := filepath.Join("/root/VirtualBox VMs", t.Name, t.Name, ".vbox") |
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.
use t.configFile here
} | ||
|
||
func (t *T) configFile() string { | ||
return filepath.Join("/root/VirtualBox VMs", t.Name, t.Name+".vbox") |
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.
use var here ?
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.
forget this comment
} | ||
|
||
func (t T) ToSync() []string { | ||
if t.Topology == topology.Failover && !t.IsShared() { |
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.
dup logic with configFiles()
} | ||
|
||
func (t *T) configFiles() []string { | ||
if !t.IsShared() && t.Topology != topology.Failover { |
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.
Single Responsibility Principle (configFiles responsibility is just return the config files)
return make([]string, 0) | ||
} | ||
|
||
func (t T) checkCapabilities() bool { |
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.
In general, all methods on a given type should have either value or pointer receivers, but not a mixture of both
https://go.dev/tour/methods/8
if !t.hasConfigFile() { | ||
return fmt.Errorf("%s not found", t.configFile()) | ||
} | ||
if err := t.start(); 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.
simplify with return t.start()
|
||
func (t T) isVmInVboxCf() (bool, error) { | ||
f, err := os.Open("/root/.config/VirtualBox/VirtualBox.xml") | ||
defer f.Close() |
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.
move this defer after err check
return err | ||
} | ||
|
||
if _, err := net.LookupIP(t.hostname()); 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.
if _, err := net.LookupIP(t.hostname()); err != nil {
t.Log().Debug().Msgf("can not do dns resolution for : %s", t.Name)
return nil
}
if err := t.waitForPing(); err != nil {
return err
}
if err := t.waitForOperational(); err != nil {
return err
}
t.Log().Info().Msgf("container %s is already down", t.Name) | ||
return nil | ||
} | ||
if err := t.containerStop(ctx); 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.
return t.containerStop(ctx)
} | ||
|
||
func (t T) waitForDown() error { | ||
t.Log().Info().Dur("timeout", *t.StopTimeout).Msgf("wait for %s shutdown, max duration %s", t.Name, *t.StopTimeout) |
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.
t.Log().Info().Dur("timeout", *t.StopTimeout).Msgf("wait for %s shutdown, max duration %s", t.Name, *t.StopTimeout)
->
t.Log().Info().Msgf("wait for %s shutdown, max duration %s", t.Name, *t.StopTimeout)
} | ||
|
||
func (t T) waitForUp() error { | ||
t.Log().Info().Dur("timeout", *t.StartTimeout).Msgf("wait for %s up", t.Name) |
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.
use same log has waitForDown
}, time.Second*2, *t.StopTimeout) | ||
} | ||
|
||
func (t T) waitForOperational() error { |
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.
dup code, refactor waitForUp, Down, Ping, Operational to
func (t *T) waitForExpectation(name, function() (bool, error) error {
return err | ||
} | ||
default: | ||
t.Log().Info().Msgf("skip stop, container state=%s", state) |
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.
perhaps add here expected state to return nil
then on default return unexpected state error
return status.Undef | ||
} | ||
|
||
if v, err := t.isVmInVboxCf(); !v && 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.
prefer err check first
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.
v, err := t.isVmInVboxCf()
if err != nil {
t.StatusLog().Error("%s", err)
return status.Undef
}
} | ||
|
||
func (t *T) Abort(ctx context.Context) bool { | ||
vUp := 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.
need refactor
t.Log().Info().Msgf("abort test: ping %s", hn) | ||
|
||
if pinger, err := ping.NewPinger(hn); err == nil { | ||
pinger.SetPrivileged(true) |
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.
dup code, perhaps create a fund (t *T)ping(d duration) bool ?
return t | ||
} | ||
|
||
func (t *T) configFile() 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.
move private func after public func ?
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.
good first pr, small change in comments
command.WithLogger(t.Log()), | ||
command.WithCommandLogLevel(zerolog.InfoLevel), | ||
command.WithStdoutLogLevel(zerolog.InfoLevel), | ||
//command.WithStderrLogLevel(zerolog.ErrorLevel), |
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.
explain why //
No description provided.