Skip to content

Commit

Permalink
addressing review
Browse files Browse the repository at this point in the history
  • Loading branch information
subnetmarco committed Sep 27, 2016
1 parent f665ae6 commit be52e29
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 12 deletions.
13 changes: 2 additions & 11 deletions kong/tools/database_cache.lua
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
local resty_lock = require "resty.lock"
local cjson = require "cjson"
local pl_tablex = require "pl.tablex"
local cache = ngx.shared.cache
local ngx_log = ngx.log

Expand Down Expand Up @@ -62,16 +63,6 @@ function _M.set(key, value)
return true
end

local function readonlytable(table)
return setmetatable({}, {
__index = table,
__newindex = function(table, key, value)
error("Attempt to modify read-only table")
end,
__metatable = false
});
end

function _M.get(key)
local value = DATA[key]

Expand All @@ -85,7 +76,7 @@ function _M.get(key)

-- Set the table as read-only
if value ~= nil and type(value) == "table" then
value = readonlytable(value)
value = pl_tablex.readonly(value)
end

return value
Expand Down
3 changes: 2 additions & 1 deletion spec/01-unit/02-conf_loader_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,12 @@ describe("Configuration loader", function()
end)
it("loads custom plugins", function()
local conf = assert(conf_loader(nil, {
custom_plugins = "hello-world,my-plugin"
custom_plugins = "hello-world,my-plugin, another-one"
}))
assert.is_nil(conf.custom_plugins)
assert.True(conf.plugins["hello-world"])
assert.True(conf.plugins["my-plugin"])
assert.True(conf.plugins["another-one"])

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Sep 27, 2016

Member

That should be a different test? This test is that we load custom_plugins. That test we now want is if we also strip their names. This is asserting 2 behaviors in the same test.

This comment has been minimized.

Copy link
@subnetmarco

subnetmarco Sep 27, 2016

Author Member

@thibaultcha thought the tests would be excessively verbose (we are testing a strip), but sure.

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Sep 27, 2016

Member

No it's fine, especially with those unit tests which are only a few LOC and fast to run.

This comment has been minimized.

Copy link
@subnetmarco

subnetmarco Sep 27, 2016

Author Member

Fixed with 9e93547

end)
it("extracts ports and listen ips from proxy_listen/admin_listen", function()
local conf = assert(conf_loader())
Expand Down

0 comments on commit be52e29

Please sign in to comment.