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

Allow DNSServer to not have proxy servers #9

Merged
merged 1 commit into from Sep 16, 2022

Conversation

azmeuk
Copy link
Contributor

@azmeuk azmeuk commented Sep 16, 2022

Fixes #8

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #9 (91c7a9c) into main (4b90d04) will decrease coverage by 0.79%.
The diff coverage is 95.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #9      +/-   ##
==========================================
- Coverage   98.30%   97.51%   -0.80%     
==========================================
  Files           5        5              
  Lines         177      201      +24     
  Branches       31       37       +6     
==========================================
+ Hits          174      196      +22     
- Misses          2        3       +1     
- Partials        1        2       +1     
Impacted Files Coverage Δ
dnserver/cli.py 94.59% <66.66%> (-5.41%) ⬇️
dnserver/main.py 99.06% <100.00%> (+0.21%) ⬆️
dnserver/version.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef85e97...91c7a9c. Read the comment docs.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

I don't think the tests needs all those changes, better to just add a new test where you create the server from scratch.

You also need to update cli.py as I think with this code, the upstream would be None by default.

dnserver/main.py Outdated Show resolved Hide resolved
dnserver/main.py Outdated Show resolved Hide resolved
dnserver/main.py Outdated Show resolved Hide resolved
tests/test_dnsserver.py Outdated Show resolved Hide resolved
tests/test_dnsserver.py Outdated Show resolved Hide resolved
@azmeuk azmeuk force-pushed the issue-8-no-upstream branch 3 times, most recently from e1c8b6e to d540a6e Compare September 16, 2022 11:40
@samuelcolvin
Copy link
Owner

let me know when this is ready to review again.

@azmeuk
Copy link
Contributor Author

azmeuk commented Sep 16, 2022

It is ready!

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM, please try to keep type hints up to date.

dnserver/cli.py Outdated
@@ -47,7 +47,7 @@ def cli_logic(args: list[str]) -> int:
parsed_args = parser.parse_args(args)

port = parsed_args.port or os.getenv('DNSERVER_PORT', None)
upstream = parsed_args.upstream or os.getenv('DNSERVER_UPSTREAM', None)
upstream = parsed_args.upstream or os.getenv('DNSERVER_UPSTREAM', DEFAULT_UPSTREAM)
Copy link
Owner

Choose a reason for hiding this comment

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

You'll need another argument --no-upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Here is how it behaves:

$ python -m dnserver ./example_zones.toml --port 5053 --no-upstream
14:47:51: starting DNS server on port 5053, upstream DNS server "None"
14:47:51: loaded 11 zone record from ./example_zones.toml, without proxy DNS server
^C14:47:57: pid=86001, got signal: SIGINT, stopping...
14:47:57: stopping DNS server

$ python -m dnserver ./example_zones.toml --port 5053 --no-upstream --upstream 8.8.8.8
14:48:03: starting DNS server on port 5053, upstream DNS server "None"
14:48:03: loaded 11 zone record from ./example_zones.toml, without proxy DNS server
^C14:48:06: pid=86088, got signal: SIGINT, stopping...
14:48:06: stopping DNS server

$ python -m dnserver ./example_zones.toml --port 5053
14:48:11: starting DNS server on port 5053, upstream DNS server "1.1.1.1"
14:48:11: loaded 11 zone record from ./example_zones.toml, with 1.1.1.1 as a proxy DNS server
^C14:48:12: pid=86152, got signal: SIGINT, stopping...
14:48:12: stopping DNS server

dnserver/main.py Outdated Show resolved Hide resolved
dnserver/main.py Outdated Show resolved Hide resolved
dnserver/main.py Outdated Show resolved Hide resolved
dnserver/main.py Outdated Show resolved Hide resolved
dnserver/main.py Outdated

def resolve(self, request, handler):
class ResolverMixin:
def resolve_(self, request, handler, records):
Copy link
Owner

Choose a reason for hiding this comment

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

if you're using a different function and passing records as an argument, this doesn't need to be a mixin at all, it can just be a plain function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

dnserver/main.py Outdated Show resolved Hide resolved
@azmeuk azmeuk force-pushed the issue-8-no-upstream branch 2 times, most recently from f5e9161 to 849bfa9 Compare September 16, 2022 12:51
@azmeuk
Copy link
Contributor Author

azmeuk commented Sep 16, 2022

This should be better now.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

dnserver/cli.py Outdated Show resolved Hide resolved
dnserver/main.py Outdated Show resolved Hide resolved
@azmeuk
Copy link
Contributor Author

azmeuk commented Sep 16, 2022

We are close :)

@samuelcolvin samuelcolvin merged commit d3b88f7 into samuelcolvin:main Sep 16, 2022
@samuelcolvin
Copy link
Owner

Great, thanks so much.

In future, please don't force push - commits are merged anyway, and force pushing makes it much harder for me to see what's changes after a review.

@azmeuk
Copy link
Contributor Author

azmeuk commented Sep 16, 2022

Ok I understand. Thank you taking time to review this.

@azmeuk azmeuk deleted the issue-8-no-upstream branch September 16, 2022 13:31
@samuelcolvin
Copy link
Owner

No problem, thanks for the contributions.

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.

Let upstream be None
2 participants