-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
feat: use query params as env #138
feat: use query params as env #138
Conversation
pkg/exporter/metrics.go
Outdated
for key, value := range params { | ||
last_value := value[len(value)-1] | ||
runEnv[key] = last_value | ||
} |
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.
HI @jonasbadstuebner, thanks for your contribution 🙂. Can we maybe check if the runEnv
map already contains a key and only add it if it is not present, to potentially break it for users which have params and environment variables with the same name?
for key, value := range params { | |
last_value := value[len(value)-1] | |
runEnv[key] = last_value | |
} | |
for key, value := range params { | |
if _, ok := runEnv[key]; !ok { | |
last_value := value[len(value)-1] | |
runEnv[key] = last_value | |
} | |
} |
Out of curiosity, why are you using the last param value and not the first one?
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.
Would it make sense to make this configurable? This would give the possibility to set default env vars but overwrite them for different scenarios, but only if explicitly stated.
To be honest, I can only guess right now, why I chose to use the last value. I don't remember anymore.
I think it was to mimic the env behavior where variables that are defined later overwrite the first ones.
Maybe joining them together with commas would make even more sense here? What do you think?
This way I could access all values in a script and not just the first one.
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 "hijacked" the ping script to show the usage of my change.
You can open http://localhost:9469/probe?script=ping&target_ips=10.0.0.0,127.0.0.1
for example and it is going to error on the first IP, even though the second one would work.
It also works with specifying the parameter multiple times, instead of already supplying a comma separated list: http://localhost:9469/probe?script=ping&target_ips=8.8.8.8&target_ips=127.0.0.1
And setting allowEnvOverwrite
to false
only pings 127.0.0.1 each time.
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.
Awesome thank you very much 🙂
e46bb35
to
611fb64
Compare
I want to access my query parameters not only by position but by name.
Merging this change will allow for scripts to read the query parameters as environment variables.
I want to deploy multiple SMons that query my script with different parameters each and the order of these parameters is something I don't want to debug.
I thought about the risk of overwriting important env variables, but this is up to the user to overwrite and shoot themselves in the foot, IMO.