-
Notifications
You must be signed in to change notification settings - Fork 174
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
Allow configuration options to be passed to module constructors #86
Comments
Seems like you are still having to change something regardless. Either the env var or the proposed cli option. |
Hi @chiptus! Hope you had a good weekend! :) I need to better understand your use case, I think, but as far as I can tell, if you had multiple containers that you wanted to set the For instance, if I was using Docker, I'd use the Am I misunderstanding things? |
In my opinion it does not really make sense to use environment variables to alter the behavior of a library. I think that it would be cleaner to just pass a parameter to the "constructor/accessor" of the library instead. Why would we want to alter the entire execution context (via an environment variable) just to alter the execution of a specific part of a program? Using an environment variable to set an option/configuration makes more sense in the context of using a program, not a library IMO. |
@deviantony I guess I disagree with you on this one -- or at least, the UNIX way is to allow software libraries (as well as programs) to be configured via environ variables. Plenty of C libraries use environs variables like Having said that, I'm certainly not opposed to doing what you suggest: having an If I'm reading you correctly, I think you're suggesting something akin to this? type Options struct {
Chroot string
}
func CPUWithOptions(opts *Options) (*CPUInfo, error) {
...
}
// envOptions returns a point to an Options struct that has been
// filled in with environs variables (such as GHW_CHROOT)
func envOptions() (*Options) {
chrootEnv := os.Getenv("GHW_CHROOT")
if chrootEnv == "" {
chrootEnv = "/"
}
return &Options{
Chroot: chroot,
}
}
func CPU() (*CPUInfo, error) {
return CPUWithOptions(envOptions())
} |
@jaypipes thanks for considering this ! Indeed that would be a neat implementation, we would then be able to use it like this: opts := ghw.Options{
Chroot: '/overrided/root',
}
pci, _ := ghw.PCIWithOptions(opts)
// cpu, _ := ghw.CPUWithOptions(opts)
// ...
pci.ListDevices() |
Yup, that's exactly what I was thinking. OK, I'll work on that. :) |
Brilliant, thanks ! |
@jaypipes, that's exactly what I was thinking. Thanks!
thanks @deviantony for communicating my thoughts :D
|
Instead of adding a So, the interface for overriding will look like this instead: cpu := ghw.CPU(ghw.WithChroot("/my/override/path")) |
Even better IMO. |
Each module (CPU(), Memory(), Network(), etc...) now supports calling the main module constructor with zero or more pointers to WithOption structs. There is only a single option (the CHROOT value for path construction) but I've designed it so the system is flexible for future additional options without needing to change the calling interface. To specify a chroot override for a particular module, use the WithChroot() function, like so: ```go mem, err := ghw.Memory(WithChroot("/host")); ``` Issue #86
It seems a bit weird to set GHW_CHROOT as env var. we might have a few containers that each of them has a different CHROOT, so would like to have the option to set it for each invocation. an env var is seems too global. Maybe you can have a options object that is passed to each of the root methods (like
ghw.CPU()
, and then check if it's missing the chroot then check for the env var and if it's missing set it to the default root.What do you say?
The text was updated successfully, but these errors were encountered: