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.

Sign up for GitHub

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

Jump to bottom

fix ipv6 display in startup info log #2285

Merged
merged 5 commits into from Oct 24, 2021
Merged

fix ipv6 display in startup info log #2285

merged 5 commits into from Oct 24, 2021

Conversation

sjsadowski
Copy link
Contributor

Resolves #2284 - checks for presence of a colon in the host, which is only legal in ipv6 address types, and brackets it accordingly

@sjsadowski sjsadowski requested a review from a team as a code owner October 24, 2021 13:57
@Tronic
Copy link
Member

Tronic commented Oct 24, 2021

Maybe tests as well. Can we run IPv6 on CI?

Tronic
Tronic requested changes Oct 24, 2021
View reviewed changes
sanic/app.py Outdated Show resolved Hide resolved
@sjsadowski
Copy link
Contributor Author

Maybe tests as well. Can we run IPv6 on CI?

What test should we add - scanning the infolog output for verification?

@Tronic
Copy link
Member

Tronic commented Oct 24, 2021

What test should we add - scanning the infolog output for verification?

Take and modify one of the existing tests that check for the Goin' Fast log entry. This alone should give full coverage on this diff and avoid the same regression happening again. No need to use the test client to do any requests (although ultimately that should be done as well, if we really have NO IPv6 tests currently).

Tronic
Tronic previously approved these changes Oct 24, 2021
View reviewed changes
Copy link
Member

@Tronic Tronic left a comment

Choose a reason for hiding this comment

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

LGTM, unless you want to add that test... But this already hits the coverage target because it is missing only half a line now :D

@sjsadowski
Copy link
Contributor Author

Take and modify one of the existing tests that check for the Goin' Fast log entry. This alone should give full coverage on this diff and avoid the same regression happening again. No need to use the test client to do any requests (although ultimately that should be done as well, if we really have NO IPv6 tests currently).

Will do. Also, looks like we don't have a test for actually specifying an ipv4 address, though we do have one for starting with default (127.0.0.1)

@codeclimate
Copy link

codeclimate bot commented Oct 24, 2021

Code Climate has analyzed commit 60b7086 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (86% is the threshold).

This pull request will bring the total coverage in the repository to 86.5% (0.0% change).

View more on Code Climate.

@sjsadowski sjsadowski merged commit 5e1ef96 into main Oct 24, 2021
@sjsadowski sjsadowski deleted the issue-2284 branch October 24, 2021 16:14
ChihweiLHBird pushed a commit to ChihweiLHBird/sanic that referenced this pull request Jun 1, 2022
@sjsadowski @ChihweiLHBird
fix ipv6 display in startup info log (sanic-org#2285)
fbac34b 
* fix ipv6 display in startup info log

* refactored to oneliner by request

* Added test for passing ipv4 host

* Added test for passing ipv6 any host

* Added test for passing ipv6 loopback host
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tronic Tronic Tronic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Goin' Fast URL IPv6 address is not bracketed
2 participants
@sjsadowski @Tronic