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

Don't return Permerror on no host or empty TXT record #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trtstm
Copy link
Contributor

@trtstm trtstm commented Jun 26, 2016

Fix for #3. But don't know if it's a good way to solve it...
The problem is that net.LookupTXT does not return a specific error if there was no TXT record. It just returns "no such host".

@maarten-postmastery
Copy link

Why not just return the underlying error?

record, err := spf.dns.GetSPFRecord(domain)
if err != nil {
    return nil, err
}

When err is nil, you should expect a valid SPF record. Policy about what to do when there is an SPF lookup error could be implemented on a higher level.

In any case, testing for error strings is bad practice. See this post and this post about inspecting errors.

@trtstm
Copy link
Contributor Author

trtstm commented Jun 28, 2016

Ye you are right, thats what I thought when reading #3. I just created this fix without considering whether this is something that should be fixed :). That's why I assigned @DenBeke to validate it.
So this pull request can be deleted if @DenBeke agrees with it.

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.

None yet

3 participants