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

Export selector and DNS response via Verification #42

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kmille
Copy link

@kmille kmille commented Mar 14, 2021

This makes debugging much easier. It can be difficult to debug issues
as the verification struct does not contain all information used for
the verification like the public key or the selector. If you
change the DNS record and don't know if DNS is still cached further
information can help.

Thank you for writing this library!

kmille

kmille added 3 commits March 14, 2021 15:31
This makes debugging much easier. It can be difficult to debug issues
as the verification struct does not contain all information used for
the verification like the public key or the selector. If you
change the DNS record and don't know if DNS is still cached further
information can help.
dkim/verify.go Outdated
@@ -79,6 +83,9 @@ type Verification struct {
// The expiration time. If the signature doesn't expire, it's set to zero.
Expiration time.Time

// The QueryResult holds the parsed DNS response
QueryResult *queryResult
Copy link
Owner

Choose a reason for hiding this comment

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

We can't add a public field with a private type like this.

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand what you mean. Why is it private and why can't we export it?

Copy link
Owner

Choose a reason for hiding this comment

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

It's unexported, because it begins with a lowercase letter.

Copy link
Author

Choose a reason for hiding this comment

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

But QueryResult starts with a big letter? Can you explain what you mean? Thanks.

Copy link
Owner

Choose a reason for hiding this comment

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

The field QueryResult is exported, the type *queryResult isn't.

Copy link
Author

Choose a reason for hiding this comment

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

ah I see. I'm still not sure about you're opinion. Should I make the type public or do you think that's a bad idea in general?

@emersion
Copy link
Owner

Can't we add more context to the error messages instead?

@kmille
Copy link
Author

kmille commented Mar 16, 2021

I want to write a mail tester like mail-tester.com. If something fails, I want to show the used public keys, selector, etc. as debug infos. I think for this use case a more verbose error message is not sufficient. Having access to all fields makes it easier.

@kmille
Copy link
Author

kmille commented Apr 3, 2021

Hey,
can you give me a status update what prevents you from merging?

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.

2 participants