-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature : AlephDNS #47
Conversation
ebb4afb
to
93ea8ad
Compare
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.
👌
Merge? Yes/No/Only with PBJ? |
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.
to Merge
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.
Sorry for reviewing late for merge, but I have a few issues with the quality of this branch that should be improved before releasing it.
async def query(self, name: str, query_type: str): | ||
try: | ||
return await self.resolver.query(name, query_type) | ||
except Exception as e: |
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.
@aliel what is the exception that we are expected here ? Catching all exceptions to stdout is not a best practice.
class AlephDNS: | ||
def __init__(self): | ||
self.resolver = aiodns.DNSResolver(servers=settings.DNS_RESOLVERS) | ||
self.fqdn_matcher = re.compile(r"https?://?") |
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.
Why the second ?
at the end ?
return None | ||
|
||
def url_to_domain(self, url): | ||
return self.fqdn_matcher.sub("", url).strip().strip("/") |
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.
You can simplify the strip()
call
return self.fqdn_matcher.sub("", url).strip(" /")
def url_to_domain(self, url): | ||
return self.fqdn_matcher.sub("", url).strip().strip("/") | ||
|
||
async def get_ipv6_address(self, url: str): |
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.
What type does this return ?
|
||
async def check_domain_configured(self, domain, target, owner): | ||
try: | ||
print("Check...", target) |
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.
Why not use logging.debug
instead of print
here ?
}, | ||
"info": f"Create a CNAME record for {domain} with value {cname_value}", | ||
"on_error": f"CNAME record not found: {domain}" | ||
}) |
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.
Would it make sense to use a dataclasses instead of a dict for this data ?
|
||
async def check_domain( | ||
self, url: str, target: str, owner: Optional[str] = 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.
Can you add a docstring to explain what this is checking ?
for _res in res: | ||
if hasattr(_res, "text") and _res.text == record_value: | ||
found = True | ||
|
||
if found == False: | ||
raise DomainConfigurationError( | ||
(dns_rule["info"], dns_rule["on_error"], status) | ||
) |
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.
Why not use a for ... else ...
loop ?
for _res in res:
if hasattr(_res, "text") and _res.text == record_value:
break
else:
raise DomainConfigurationError(
(dns_rule["info"], dns_rule["on_error"], status)
)
* Feature : AlephDNS add instances support add ipfs support add program support --------- Co-authored-by: aliel <[email protected]>
* Feature : AlephDNS add instances support add ipfs support add program support --------- Co-authored-by: aliel <[email protected]>
* Feature : AlephDNS (#47) * Feature : AlephDNS add instances support add ipfs support add program support --------- Co-authored-by: aliel <[email protected]> * Fix: Reformat with `black` * minor change on dns settings * avoid not valid local variable in certain conditions * add method to retreive txt values * fix dns record check * fix program cname value * Refactor: str -> Enum for DNS target * Refactor: Add types to arguments * Fix: variable did not exist * Add typing * Cleanup: imports with isort * Cleanup: typing * Refactor: domain_from_url * Refactor: use a generator * Cleanup: Add docstring and property * WIP: Refactor and cleanup domain related code * [dns] let checks continue * fix hostname_from_url when it's already a hostname * remove unused import * speedup dns detection using authoritative ns server * fix cname to target * fix cond * fix while loop * Fix typing; add docs; refactor for clarity --------- Co-authored-by: 1yam <[email protected]> Co-authored-by: aliel <[email protected]> Co-authored-by: mhh <[email protected]>
Implement AlephDNS from @aliel inside SDK