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

πŸ”₯ adds and additional line to starting banner "bound: http://0.0.0.0:8080" #1210

Merged
merged 6 commits into from
Mar 9, 2021

Conversation

gregchalmers
Copy link
Contributor

I submitted an issue #1207 saying when I bound to socket "0.0.0.0" the banner said 127.0.0.1.
i.e.

 β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β” 
 β”‚                    Fiber v2.5.0                   β”‚ 
 β”‚               http://127.0.0.1:3000               β”‚ 
 β”‚                                                   β”‚ 
 β”‚ Handlers ............. 0  Processes ........... 1 β”‚ 
 β”‚ Prefork ....... Disabled  PID ............ 382963 β”‚ 
 β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜ 

with this PR you will now see the following when host == "0.0.0.0" or ""

 β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β” 
 β”‚                    Fiber v2.5.0                   β”‚ 
 β”‚              https://127.0.0.1:38171              β”‚ 
 β”‚            bound https://0.0.0.0:38171            β”‚ 
 β”‚                                                   β”‚ 
 β”‚ Handlers ............. 0  Processes ........... 1 β”‚ 
 β”‚ Prefork ....... Disabled  PID ............ 384749 β”‚ 
 β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜ 

I'm just a newbie at go so it might be worth a review, or a rejection. I tested manually and go test

@welcome
Copy link

welcome bot commented Mar 9, 2021

Thanks for opening this pull request! πŸŽ‰ Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@koddr
Copy link
Contributor

koddr commented Mar 9, 2021

@gregchalmers and @kiyonlin

I think it would be a good idea to color-code the clickable address. Or make this notation for more understandable view:

β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β” 
β”‚                    Fiber v2.5.0                   β”‚ 
β”‚               http://127.0.0.1:8080               β”‚ 
β”‚       (bound on host 0.0.0.0 and port 8080)       β”‚ 
β”‚                                                   β”‚ 
β”‚ Handlers ............. 6  Processes ........... 1 β”‚ 
β”‚ Prefork ....... Disabled  PID ............ 321776 β”‚ 
β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜ 

@kiyonlin
Copy link
Contributor

kiyonlin commented Mar 9, 2021

I love @koddr's idea

@gregchalmers
Copy link
Contributor Author

thanks @koddr I updated the message, yeah its way clearer.

Copy link
Contributor

@kiyonlin kiyonlin left a comment

Choose a reason for hiding this comment

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

LGTM

@koddr
Copy link
Contributor

koddr commented Mar 9, 2021

By the way, @kiyonlin

Maybe it is better to use the switch operator in this case than if...else ? I mean, do it like this:

	// ...
	switch host {
	case "0.0.0.0":
		// logic with 0.0.0.0
	default:
		// default case
	}

We may still have other cases and it will look better this approach (in the future). What do you think?

@kiyonlin
Copy link
Contributor

kiyonlin commented Mar 9, 2021

I think we only have one special case 0.0.0.0. So it is not very necessary imo. We could use switch if more cases happen.

@koddr
Copy link
Contributor

koddr commented Mar 9, 2021

I think we only have one special case 0.0.0.0. So it is not very necessary imo. We could use switch if more cases happen.

Okay! So, LGTM now.

@Fenny Fenny merged commit 789bf38 into gofiber:master Mar 9, 2021
@welcome
Copy link

welcome bot commented Mar 9, 2021

Congrats on merging your first pull request! πŸŽ‰ We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

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

5 participants