-
Notifications
You must be signed in to change notification settings - Fork 170
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
Modular #92
Modular #92
Conversation
if resolver is 'nmc' and not VALID_NMC_DOMAINS.test path | ||
@log.debug gLineInfo "ignoring request for: #{path}" | ||
return notFound() | ||
@dnschain.chains['namecoin'] |
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.
Return error 400
as per #87 instead.
['none'] | ||
|
||
if not (resolver = @dnschain.chains[resolverName]) | ||
@log.warn gLineInfo, 'unknown blockchain', {host: req.headers.host, blockchainHeader: req.headers.blockchain, remoteAddress: req.connection.remoteAddress} |
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.
gLineInfo
is a function that needs to be called (often with some argument, in this case 'unknown blockchain'
would be that argument). Double-check all your other uses of gLineInfo
to make sure that you're doing it correctly.
gConf.get('dns:externalIP'), gExternalIP() | ||
_.map(stores, (c) -> | ||
c.get('host'))... | ||
].filter (o)-> typeof(o) is 'string' |
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.
This doesn't seem like the right place to put this... and it should be called localhosts
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.
OK, you can keep it here since it's semi-configurable (since it's referencing the config vars), however:
- Needs to be renamed as
localhosts:
- Pretty sure you can't use
gConf
, it will be undefined. You can useconfig.get
instead ofgConf.get
@wemeetagain OK, consider my first review complete for now, feel free to make relevant changes (please please test them however you can) and send another commit. I'll review it and if all is good we can merge this in to |
set gConf.localhosts once added dnschain.chainsTLDs which is dnschain.chains mapped to each chain TLD use dnschainsTLDs instead of regex
else | ||
@log.warn gLineInfo "unknown host type: #{req.headers.host} -- defaulting to namecoin.dns!" | ||
'nmc' | ||
['none'] |
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 think this can be simplified to:
[...,resolverName] =
if S(header = req.headers.blockchain || req.headers.host).endsWith('.dns')
S(header).chompRight('.dns').s.split('.')
else
['none']
Since #105 is ahead of this am closing. |
No description provided.