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

cleanup unused variables and change Clab.WithRuntime(...) to use *runtime.RuntimeConfig #502

Merged
merged 1 commit into from
Jul 9, 2021

Conversation

steiler
Copy link
Collaborator

@steiler steiler commented Jul 8, 2021

the Clab bound options where not used any more. Hence I've deleted them.
changed Clab.WithRuntime from (name string, d bool, dur time.Duration, gracefulShutdown bool) to (name string, rtconfig *runtime.RuntimeConfig).

Timeout: dur,
Debug: d,
}),
runtime.WithConfig(rtconfig),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can discuss if we should use a copy of the runtimeConfig provided to avoid any race conditions.
On the other hand one could use that handle to change runtime config by intention.
Unsure myself.

@hellt
Copy link
Member

hellt commented Jul 8, 2021

  1. as to the removed options, we need to see why they were not used anymore. Both graceful shutdown and debug were used before
  2. now when I see the full PR I am not sure it is a good idea to import runtime package for cmd package
    wdyt @karimra ?

@karimra
Copy link
Member

karimra commented Jul 8, 2021

  1. I think the values are still used, they just moved to runtimeConfig
  2. I think it's ok, as long as runtime and clab do not import cmd, it should be cycle free

@steiler
Copy link
Collaborator Author

steiler commented Jul 8, 2021

Also the debug variable, is just consumed in one case by the docker runtime.

if c.config.Debug {
log.Debugf("network '%s' has %d active endpoints, deletion skipped", c.Mgmt.Network, numEndpoints)
for _, endp := range nres.Containers {
log.Debugf("'%s' is connected to %s", endp.Name, network)
}
}

And even this can be left out, since this is omitted via the debug level, which even used the global debug var.
To be honest that flag is not really needed atm.

@hellt
Copy link
Member

hellt commented Jul 9, 2021

Also the debug variable, is just consumed in one case by the docker runtime.

the debug is used not only in docker, it is also used in ignite

vmChans, err := operations.StartVMNonBlocking(vm, c.config.Debug)

@hellt
Copy link
Member

hellt commented Jul 9, 2021

but the rest looks ok to me
Building the binary with -race doesn't detect any issues with passing runtime config as a pointer, so should be ok as well

@hellt hellt merged commit e47dcd5 into master Jul 9, 2021
@hellt hellt deleted the clabwithconf branch July 9, 2021 10:00
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.

None yet

3 participants