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

Offer a way to avoid using os.GetEnv #1438

Closed
atoulme opened this issue Mar 26, 2023 · 7 comments · Fixed by #1439
Closed

Offer a way to avoid using os.GetEnv #1438

atoulme opened this issue Mar 26, 2023 · 7 comments · Fixed by #1439

Comments

@atoulme
Copy link
Contributor

atoulme commented Mar 26, 2023

Is your feature request related to a problem? Please describe.
When using gopsutil programmatically, we want to avoid relying on environment variables. We have use cases where we want to find information about processes in the container and outside the container, for example.
Describe the solution you'd like
We want to keep the current default behavior of reading from os.GetEnv. However, we also want a way to override optionally this approach by passing a map as part of the invocation.

3 approaches to this:

  • set os.GetEnv as a function defined as a public var in the gopsutil common package, and allow users to access and override it. This is not thread-safe and probably just a quick hack to get by.
  • Add a map of values to all calls to gopsutil as an additional argument. Keep the current API, making sure it passes in os.GetEnv as its default parameter value.
  • Use a context parameter. The context parameter may optionally have a key that points to a custom function, that can be used instead of the default os.GetEnv.

Describe alternatives you've considered
Still looking.

Additional context
N/A

@shirou
Copy link
Owner

shirou commented Apr 1, 2023

Thank you for the issue and implementation. And sorry for the late reply.

Personally, I don't like to give various additional information to the context and pass it on. Because the value of context has no type and it is difficult to use. (Also, since gopsutil was developed before context was introduced, some functions do not have context as an argument)

I thought it would be better to pass a argument as I wrote in #1420 (comment). and I think there are also a way to add variable length arguments.

type option struct {
	HostSys string
	HostEtc string
	HostVar string
}

type Option interface {
	Apply(*option)
}

func somefunc() {
	Load(ctx, common.WithHostSys("foo"), common.WithHostEtc("bar"))
}

func Load(ctx context.Context, options ...Option) {
}

However, after I read your implementation, I am beginning to think that if we are providing custom functions and using read-only values, context would be fine.

Perhaps you will recommend context, but what are your thoughts on this variable length argument approach?

@atoulme
Copy link
Contributor Author

atoulme commented Apr 1, 2023

The problem is how much you'll need to change APIs to pass the options around. The context object has the benefit of being there already, for most of the methods.

@shirou
Copy link
Owner

shirou commented Apr 1, 2023

To my understanding, adding a variable length argument does not change the API.

@atoulme
Copy link
Contributor Author

atoulme commented Apr 6, 2023

I see what you mean now. Let me try this approach. Thanks!

@shirou
Copy link
Owner

shirou commented Apr 6, 2023

To clarify my thoughts, I am now inclined to the idea that it is better to use context. However, perhaps the previous method of using variable length arguments might be a better way to go. If you have a time, you don't have to do the whole implementation, just have them try it out and then let us know your thoughts.

@atoulme
Copy link
Contributor Author

atoulme commented Apr 12, 2023

I'm fresh out of time but will look into it for an hour.

@atoulme
Copy link
Contributor Author

atoulme commented Apr 12, 2023

I looked into it and found that we already use a vararg for a number of methods:

func HostProc(combineWith ...string) string {
	return GetEnv("HOST_PROC", "/proc", combineWith...)
}

func HostSys(combineWith ...string) string {
	return GetEnv("HOST_SYS", "/sys", combineWith...)
}

func HostEtc(combineWith ...string) string {
	return GetEnv("HOST_ETC", "/etc", combineWith...)
}

func HostVar(combineWith ...string) string {
	return GetEnv("HOST_VAR", "/var", combineWith...)
}

func HostRun(combineWith ...string) string {
	return GetEnv("HOST_RUN", "/run", combineWith...)
}

func HostDev(combineWith ...string) string {
	return GetEnv("HOST_DEV", "/dev", combineWith...)
}

func HostRoot(combineWith ...string) string {
	return GetEnv("HOST_ROOT", "/", combineWith...)
}

All those methods would need to move to use a []string parameter instead. Since some of the method calls don't use combinations, you end up having to field all those calls with HostRoot(nil, opts...) which I think is confusing.

I am in favor of the context PR. Please review and let me what you think of the comments from @rmfitzpatrick.

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 a pull request may close this issue.

2 participants