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

fix(cli) properly look for serf in known paths #1997

Merged
merged 1 commit into from
Feb 7, 2017
Merged

Conversation

subnetmarco
Copy link
Member

@subnetmarco subnetmarco commented Jan 18, 2017

Summary

Our distributions put the serf executable in a known path at /usr/local/bin/serf. Adds this path as a known path (like we do for resty) when searching for Serf, if serf_path is not specified in the configuration file.

Full changelog

  • Look for serf in known paths, if serf_path is not specified.

Issues resolved

Fix #1953

log.debug("searching for 'serf' executable")

if kong_config.serf_path then
serf_search_paths = { kong_config.serf_path }
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't override all of the other potential locations. Maybe it should be replaced with:

table.insert(serf_search_paths, 1, kong_config.serf_path)

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it - if I explicitly tell Kong to look into that path and it's not found, shouldn't an error be returned even if it's found somewhere else?

Does the serf_path property mean:

  • search into serf_path in addition to the standard paths?
  • look explicitly into serf_path and if it doesn't exist, return an error?

Seems like (2) would be a more expected behavior?

Copy link
Member

Choose a reason for hiding this comment

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

(1) would be a more expected behavior, similar to the shell's $PATH but also to the Lua's LUA_PATH and LUA_CPATH properties, also configurable from the Kong configuration.

Copy link
Member

Choose a reason for hiding this comment

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

It's also less error prone.

Copy link
Member

Choose a reason for hiding this comment

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

in other cases we said that if the user specifies a value, then not being able to honor that value is an error. We've had this discussion before.

If I would specify an argument "xyz" and the application starts and ends up running with "123" instead, that would be bad imo.

so it ought to be the nr 2. My 2cts.

Copy link
Member

Choose a reason for hiding this comment

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

This goes against all the other path variables in the shell and this project -_-

@thibaultcha thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Jan 24, 2017
@Tieske Tieske merged commit 64f5e9b into release/0.10.0 Feb 7, 2017
@Tieske Tieske deleted the fix/cli-serf branch February 7, 2017 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants