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

Avoid nameserver duplication #636

Merged
merged 4 commits into from
Mar 1, 2018
Merged

Conversation

maneta
Copy link

@maneta maneta commented Feb 28, 2018

correct server == resolver case in #630

@@ -179,11 +179,12 @@ nameserver 127.0.0.1
end)

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

@mikz mikz Mar 1, 2018

Choose a reason for hiding this comment

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

I think this deserves a new test. Because it basically changes it so we don't have a test for the order of returned nameservers from a normal file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Also, I think that we have similar problems with other tests. For example, this test is checking that commented lines are ignored, but there isn't an it clause explaining that or an explicit assertion.

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 e2755eb

--- user_files
>>> resolv.conf
nameserver 127.0.1.1
nameserver 1.2.3.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at the end of the file. Would be good to set-up your editor to always keep a new-line at the EOF.

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 3afea98

@maneta maneta merged commit 7cdca78 into master Mar 1, 2018
@maneta maneta deleted the fix-duplicity-nameserver-resolver branch March 1, 2018 14:52
@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

- 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)
- Avoid `nameserver` repetion from `RESOLVER` variable and `resolv.conf` file [PR #636](https://github.com/3scale/apicast/pull/636)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

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

it('do not replicates nameservers from resolver env var', function()
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@davidor
Copy link
Contributor

davidor commented Mar 1, 2018

👍

Thanks for your contribution @maneta

@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