Split resolv.conf in lines #618
Conversation
maneta
force-pushed
the
fix-resolv-conf-parser
branch
9 times, most recently
from
3f2de36
to
e016852
Compare
gateway/src/resty/resolver.lua
Outdated
| insert(nameservers, nameserver.new(server)) | ||
| end | ||
|
|
||
| for line in gmatch(resolv_conf, '(.*)\n]') do |
There was a problem hiding this comment.
I think this could be better as:
for _,line in ipairs(re.split(resolv_conf, [[\n+$]])) doThere was a problem hiding this comment.
And actually I think it would be nice to extract parsing of the file into own function so it can be unit tested. I can do that if you want.
Also looks like there is similar bug on line 116 in this file. search would also be extracted from a comment line.
There was a problem hiding this comment.
@mikz go ahead this was just me playing around with lua regex. But lua was winning me :)
There was a problem hiding this comment.
If you want I'm happy to provide guidance and let you do it ;)
There was a problem hiding this comment.
yay! Thanks @mikz I will do it then
maneta
force-pushed
the
fix-resolv-conf-parser
branch
8 times, most recently
from
5b4452d
to
999317e
Compare
| @@ -34,6 +34,15 @@ GET /t | |||
| nameservers: 3 127.0.1.15353 1.2.3.453 4.5.6.753 | |||
| --- user_files | |||
| >>> resolv.conf | |||
| # nameserver updated in comentary | |||
There was a problem hiding this comment.
This is good. It's testing more things than before, so I think it's worth expanding the description of the test (line 16).
spec/resty/resolver_spec.lua
Outdated
| @@ -163,6 +163,14 @@ describe('resty.resolver', function() | |||
| before_each(function() | |||
| tmpname = io.tmpfile() | |||
|
|
|||
| tmpname:write('# nameserver updated in comentary\n') | |||
There was a problem hiding this comment.
How about calling write just once? I think it looks better:
tmpname:write(
[[# nameserver updated in comentary\n
#nameserver updated in comentary\n
#comentary nameserver\n
..............]])(I think the '\n' need to be there, but better test it to be sure)
|
Travis tests are failing because https://olivinelabs.com/busted/ is down, so don't worry about those. |
| -- TODO: implement port matching based on https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=549190 | ||
| if server ~= resolver then | ||
| if server and server ~= resolver then |
There was a problem hiding this comment.
I think we don't have tests for the case where server == resolver.
maneta
force-pushed
the
fix-resolv-conf-parser
branch
from
646091f
to
19719b8
Compare
|
@maneta the changelog entry is not under the correct version. It should be under |
maneta
force-pushed
the
fix-resolv-conf-parser
branch
2 times, most recently
from
9e491a7
to
97470e0
Compare
|
Got it @davidor now it should be good to go :) |
|
@maneta there are conflicts with master. You'll need to rebase/merge master branch. |
Calling Write just once Expanding the t/resolver.t TEST 1 documentation Updating changelog for spliting `resolv.conf` in lines
maneta
force-pushed
the
fix-resolv-conf-parser
branch
from
97470e0
to
4dcf490
Compare
|
@mikz Rebased |
| @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
| ## Fixed | |||
|
|
|||
| - Error loading policy chain configuration JSON with null value [PR #626](https://github.com/3scale/apicast/pull/626) | |||
| - Splitted `resolv.conf` in lines,to avoid commented lines [PR #618](https://github.com/3scale/apicast/pull/618) | |||
There was a problem hiding this comment.
Just a nitpick. Changelog should focus on "what" and not "how".
Here would be better fit: Skip lines starting with a comment when parsing resolv.conf
|
Thank you @maneta ! Much appreciated. |
So the code does not brake like with comented #nameserver lines