-
Notifications
You must be signed in to change notification settings - Fork 39
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
roles: introduce role for http servers #197
Conversation
a9c76da
to
0c6784e
Compare
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.
Thank you for the patch! I think we may improve it in a couple of ways.
Let's also add a public API call for this functionality in the scope of the task, so that there is no need to access internal structures directly: |
04cfd9b
to
246dc67
Compare
Updated the PR according to the comments. |
dcf425f
to
d410fae
Compare
6eb9a58
to
54efff0
Compare
CI is finally green, all comments are fixed. |
c4036a9
to
2e3dbd2
Compare
Updated the PR according to comments. |
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.
Thank you for the patch, just few non-critical notes.
### Added | ||
|
||
- `roles.httpd` role to configure one or more HTTP servers (#196) | ||
- `httpd:delete(name)` method to delete named routes (#197) |
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 have never seen anyone using name
option for a route. Until now, I didn't know it even exists. As far as I understand, it actually works not only with named routes, but with unnamed ones as well by using path as a key?
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.
No, we can't use just path. Routes are stored as opts
(which is a table of many fields, some of them are filled inside of add_route
function).
Lines 1248 to 1259 in 2e3dbd2
if opts.name ~= nil then | |
if opts.name == 'current' then | |
error("Route can not have name 'current'") | |
end | |
if self.iroutes[ opts.name ] ~= nil then | |
errorf("Route with name '%s' is already exists", opts.name) | |
end | |
table.insert(self.routes, opts) | |
self.iroutes[ opts.name ] = #self.routes | |
else | |
table.insert(self.routes, opts) | |
end |
And those opts
stored in the map with the name
as a key. If there are no name
, then we can't get opts
.
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.
Oh, I see that metrics-export-role sets the name as well
https://github.com/tarantool/metrics-export-role/blob/36130b73c0a5975ff26f0c26f78ee7a368c1207b/roles/metrics-export.lua#L265
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.
Yes, it is. It uses name, BUT we could delete a route by path too. I don't see any problems.
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.
The only problem is to create a nice and non-confusing API for the call. Maybe:
httpd:delete(path, name)
httpd:delete(path)
httpd:delete(nil, name)
But it looks a little strange.
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.
Yes, it is. It uses name, BUT we could delete a route by path too. I don't see any problems.
I'm not sure, but as far as I remember that httpd has that weird thing that allows to set multiple handlers on the same route since route is not a unique identificator (and only one of handlers will be invoked).
5a16f8a
to
ab288f1
Compare
Updated everything according to comments. |
This patch adds `roles.httpd`. This role allows to configure one or more HTTP servers. Those servers could be reused by several other roles. Servers could be accessed by their names (from the config). `get_server(name)` method returns a server by its name. If `nil` is passed, default server is returned. The server is default, if it has `DEFAULT_SERVER_NAME` (`"default"`) as a name. Closes #196
This patch adds a `httpd:delete(name)` method to delete named routes.
ab288f1
to
edcb472
Compare
Updated everything according to comments. |
### Added | ||
|
||
- `roles.httpd` role to configure one or more HTTP servers (#196) | ||
- `httpd:delete(name)` method to delete named routes (#197) |
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.
Yes, it is. It uses name, BUT we could delete a route by path too. I don't see any problems.
I'm not sure, but as far as I remember that httpd has that weird thing that allows to set multiple handlers on the same route since route is not a unique identificator (and only one of handlers will be invoked).
This patch adds
roles.http_server
. This role allows to configurate one or more HTTP servers. Those servers could be reused by several other roles.Servers could be accessed by their names (from the config).
get_server(name)
method returns a server by its name. Ifnil
is passed, default server is returned. The server is default, if it hasDEFAULT_SERVER_NAME
("default"
) as a name.This patch also adds a
httpd:delete(name)
method to delete named routes.Closes #196