feat(admin) sugar method to delete upstream target #2256
Conversation
feat(admin) get a list of active targets per upstream
| local target | ||
|
|
||
| before_each(function() | ||
| assert(helpers.dao.targets:insert { |
There was a problem hiding this comment.
shouldn't an upstream be created here? reusing an existing one might interfere with the test results here.
There was a problem hiding this comment.
But it doesn't :)
My understanding of before_each means that it runs before each nested test, meaning that we recreate the upstream and shut down kong after every test. And actually, the anti-pattern that I used in the previous PR to create upstream3 is unnecessary as well.
Nevertheless, I'll add it.
kong/api/routes/upstreams.lua
Outdated
| balancer.clean_history(self.params.upstream_id, dao_factory) | ||
|
|
||
| -- this is just a wrapper around POSTing a new target with weight=0 | ||
| local target = self.params |
There was a problem hiding this comment.
It would be more explicit to use self.upstream here
There was a problem hiding this comment.
Hmm? self.params represents the target (a table with a target key), not an upstream.
There was a problem hiding this comment.
self.params represents the request parameters, with modifications from the before handler.
I'd consider it cleaner to create a new target table here
local data, err = dao_factory.targets:insert( {
target = params.target,
upstream_id = self.upstream.id,
weight = 0,
})Then line 109 above can be dropped as well.
| return app_helpers.yield_error(err) | ||
| end | ||
|
|
||
| return responses.send_HTTP_NO_CONTENT() |
There was a problem hiding this comment.
Wondering if we should not send HTTP 201 still here. Yes, this is a DELETE and stays consistent with the rest of the API, but it isn't a traditional DELETE endpoint. It is known that this is a sugar method. Might bring confusion, but might also raise awareness about targets. However, this is mostly internal implementation details, so probably good to hide it away.
There was a problem hiding this comment.
I think if we were to return 201 here, we might as well not bother with this method at all ;)
tbh, i dont even think this is a valid PR. I don't think adding this endpoint makes it any easier for consumers to work with the upstream/target interface. But that's just my $0.02 ;)
thibaultcha
added
the
pr/changes requested
p0pr0ck5
force-pushed
the
feat/delete-target
branch
from
8ca5ea3 to
830c95f
Compare
p0pr0ck5
added
pr/status/needs review
and removed
pr/changes requested
kong/api/routes/upstreams.lua
Outdated
| " removed ", tostring(cnt), " target entries") | ||
| end | ||
| end | ||
| balancer.clean_history(self.params.upstream_id, dao_factory) |
There was a problem hiding this comment.
I'm having doubts on whether we should be moving this code. So far (at least the way I see it) the code for the management api and the Kong runtime have remained fairly separate. By moving this code into the balancer module, we change that, as this code is strictly management api.
Not sure though...
@thibaultcha wat do you think?
There was a problem hiding this comment.
Wasn't sure about the style of putting local functions in routes files, only reason I thought to put it in kong/core/balancer. Ill move it back :)
kong/api/routes/upstreams.lua
Outdated
| balancer.clean_history(self.params.upstream_id, dao_factory) | ||
|
|
||
| -- this is just a wrapper around POSTing a new target with weight=0 | ||
| local target = self.params |
There was a problem hiding this comment.
self.params represents the request parameters, with modifications from the before handler.
I'd consider it cleaner to create a new target table here
local data, err = dao_factory.targets:insert( {
target = params.target,
upstream_id = self.upstream.id,
weight = 0,
})Then line 109 above can be dropped as well.
p0pr0ck5
force-pushed
the
feat/delete-target
branch
from
830c95f to
a8b3170
Compare
Tieske
added
pr/ready (but hold merge)
|
As this is fully backward compatible, this could as well go in |
|
tagged them both for milestone |
No description provided.