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.

Sign up for GitHub

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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions kong/cmd/utils/serf_signals.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,46 @@ local serf_event_name = "kong"
local serf_version_pattern = "^Serf v([%d%.]+)"
local serf_compatible = version.set(unpack(meta._DEPENDENCIES.serf))
local start_timeout = 5
local serf_search_paths = {
"serf",
"/usr/local/bin/serf"
}

local function check_serf_bin(kong_config)
log.debug("checking 'serf' executable from 'serf_path' config setting")

local cmd = fmt("%s version", kong_config.serf_path)
local function check_version(path)
local cmd = fmt("%s version", path)
local ok, _, stdout = pl_utils.executeex(cmd)
log.debug("%s: '%s'", cmd, pl_stringx.splitlines(stdout)[1])
if ok and stdout then
local version_match = stdout:match(serf_version_pattern)
if not version_match or not serf_compatible:matches(version_match) then
return nil, "incompatible serf found. Kong requires version "..
tostring(serf_compatible)..", got "..version_match
return nil, fmt("incompatible serf found at '%s'. Kong requires version '%s', got '%s'",
path, tostring(serf_compatible), version_match)
end
return true
end
end

local function check_serf_bin(kong_config)
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 -_-

end

local found
for _, path in ipairs(serf_search_paths) do
if check_version(path) then
found = path
log.debug("found 'serf' executable at %s", found)
break
end
end

if not found then
return nil, "could not find 'serf' executable (is 'serf_path' correctly set?)"
end

return nil, "could not find 'serf' executable (is 'serf_path' correctly set?)"
return found
end

local _M = {}
Expand Down