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

add a hostname verification function that doesn't do dns resolution #142

Merged

Conversation

alexanderkjall
Copy link
Contributor

This adds a hostname check that doesn't do a dns verification of the hostname, in order to support the use case of tracking assets that are behind a split horizon dns setup.

This is part 1 of 2, the other part will be in vulcan-api and use this function.

types.go Outdated
@@ -215,6 +215,21 @@ func IsHostname(target string) bool {
return len(r) > 0
}

// IsHostnameNoDnsResolution returns true if the target is not an IP.
func IsHostnameNoDnsResolution(target string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would do: s/Dns/DNS in the name of the 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.

fixed :)

types_test.go Outdated
@@ -628,6 +628,45 @@ func TestIsHostname(t *testing.T) {
}
}

func TestIsHostnameNoDnsResolution(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would do: s/Dns/DNS in the name of the 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.

fixed :)

@kozmic
Copy link

kozmic commented Mar 6, 2024

@manelmontilla Any chance on getting this reviewed?

@kozmic
Copy link

kozmic commented Mar 25, 2024

@manelmontilla : Is there anything we can do to help get this merged?

Copy link
Contributor

@manelmontilla manelmontilla left a comment

Choose a reason for hiding this comment

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

LGTM

@manelmontilla
Copy link
Contributor

Hi @kozmic,
Sorry for the delay.
Everything is okay, the only thing needed before merging is to rebase the branch.

@alexanderkjall alexanderkjall force-pushed the check-hostname-without-dns-resolution branch from cb055d0 to 868061d Compare April 2, 2024 16:16
@alexanderkjall
Copy link
Contributor Author

rebased

@jesusfcr jesusfcr merged commit 24c0e53 into adevinta:master Apr 4, 2024
1 check passed
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.

4 participants