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.

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 resolv.conf search parsing #635

Merged
merged 1 commit into from
Mar 6, 2018
Merged

Conversation

maneta
Copy link

@maneta maneta commented Feb 28, 2018

Corrects parsing search local.domain #foobar in #630

@maneta maneta requested review from mikz and davidor February 28, 2018 21:38
@maneta maneta changed the title Fix resolvconf search regex Fix resolv.conf search parsing Feb 28, 2018
Makefile Outdated
busted: dependencies $(ROVER) ## Test Lua.
@$(ROVER) exec bin/busted
BUSTED_FILES ?=
busted: dependencies $(ROVER) ## Test Lua. To test a specific file: make busted BUSTED_FILES=spec/resty/testname.lua
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidor now you can select which busted test to execute from the Makefile too :)

Makefile Outdated
busted: dependencies $(ROVER) ## Test Lua.
@$(ROVER) exec bin/busted
BUSTED_FILES ?=
busted: dependencies $(ROVER) ## Test Lua. To test a specific file: make busted BUSTED_FILES=spec/resty/testname.lua
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sold. I don't think this needs to be documented.
If you are skilled enough to want to run just one test you'll just run bin/busted without the makefile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 72f9a66

Makefile Outdated
@@ -61,7 +61,7 @@ endif
prove: HARNESS ?= TAP::Harness
prove: PROVE_FILES ?= $(shell find t examples -type f -name "*.t")
prove: export TEST_NGINX_RANDOMIZE=1
prove: $(ROVER) nginx cpan ## Test nginx
prove: $(ROVER) nginx cpan ## Test nginx. To execute a specific test: make prove PROVE_FILES=t/testname.t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here to. I don't intend PROVE_FILES to be documented and used by public.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 72f9a66

search localdomain.example.com local
nameserver 127.0.0.2
nameserver 127.0.0.1
search localdomain.example.com local #search nameserver
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have created a new test for this instead of modifying the existing one.

I think that having it('ignores comments after search or nameserver', function() ...) would make the tests cleaner. What do you think?

Copy link
Author

@maneta maneta Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok @davidor so maybe create two more like one for parsing a file without commentaries and other for commentaries after nameserver and search?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done a339d4b

@maneta maneta force-pushed the fix-resolvconf-search-regex branch from a339d4b to c052e30 Compare March 1, 2018 11:32
Makefile Outdated
busted: dependencies $(ROVER) ## Test Lua.
@$(ROVER) exec bin/busted
BUSTED_FILES ?=
busted: dependencies $(ROVER) ## Test Lua.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 3c177e4

Makefile Outdated
@@ -61,7 +61,7 @@ endif
prove: HARNESS ?= TAP::Harness
prove: PROVE_FILES ?= $(shell find t examples -type f -name "*.t")
prove: export TEST_NGINX_RANDOMIZE=1
prove: $(ROVER) nginx cpan ## Test nginx
prove: $(ROVER) nginx cpan ## Test nginx.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another trailing whitespace.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 3c177e4

@@ -176,10 +195,25 @@ search localdomain.example.com local
nameserver 127.0.0.2
nameserver 127.0.0.1
]])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunch of whitespace here. We probably should have some linter for that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I'm trying to get this vscode right but I think I will just go back to VI

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 3c177e4

@maneta maneta force-pushed the fix-resolvconf-search-regex branch from c052e30 to 3c177e4 Compare March 1, 2018 14:58
Makefile Outdated
busted: dependencies $(ROVER) ## Test Lua.
@$(ROVER) exec bin/busted
BUSTED_FILES ?=
busted: dependencies $(ROVER) ## Tezs Lua.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

]])
end)

it('returns nameserver touples', function()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tuples

assert.same({ '127.0.0.1', 53 }, nameservers[2])
end)

it('returns nameserver touples ignoring commentaries', function()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tuples

@maneta maneta force-pushed the fix-resolvconf-search-regex branch from 433fd58 to 10c3178 Compare March 6, 2018 15:13
- create more test cases for it
- let `make busted` select test files
@maneta maneta force-pushed the fix-resolvconf-search-regex branch from 10c3178 to a8daad4 Compare March 6, 2018 16:16
@maneta maneta merged commit 02868a4 into master Mar 6, 2018
@maneta maneta deleted the fix-resolvconf-search-regex branch March 6, 2018 16:52
@mikz mikz mentioned this pull request Jun 11, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants