maxcnunes / waitforit

Wait until an address become available.
MIT License
172 stars 26 forks source link

For http(s) address, port is now required #26

Closed nsteinmetz closed 5 years ago

nsteinmetz commented 6 years ago

Hi,

Small regression for https urls - it's now mandatory to specify the port (443) to the url whereas it worked without so far.

./waitforit -address=https://www.google.fr -debug                                                                                                                                                            
2018/09/28 08:45:18 Waiting 10 seconds
2018/09/28 08:45:18 Ping host: www.google.fr
2018/09/28 08:45:18 Ping host: www.google.fr
2018/09/28 08:45:18 Down: www.google.fr
2018/09/28 08:45:18 dial tcp: address www.google.fr: missing port in address

With port defined:

./waitforit -address=https://www.google.fr:443 -debug                                                                                                                                             
2018/09/28 08:45:41 Waiting 10 seconds
2018/09/28 08:45:41 Ping host: www.google.fr:443
2018/09/28 08:45:41 Ping host: www.google.fr:443
2018/09/28 08:45:41 Up: www.google.fr:443
2018/09/28 08:45:41 Ping http address: https://www.google.fr:443
2018/09/28 08:45:41 Ping http address https://www.google.fr:443 200 OK

Will investigate later today.

Nicolas

nsteinmetz commented 6 years ago

Think it's between here:

https://github.com/maxcnunes/waitforit/blob/master/connection.go#L59-L67

and here:

https://github.com/maxcnunes/waitforit/blob/master/network.go#L97

I tend to think that Host is overwritten - should it be splitted into two variables as you may want to ping Host one one side but do a http call on <scheme>/<host>:<port>

In valid_config.json for tests, you should add also:

    {
      "address": "http://google.com",
      "timeout": 40
    },    
    {
      "address": "https://google.com",
      "timeout": 40
    }, 
nsteinmetz commented 6 years ago

hi @maxcnunes,

Any idea on this ? Too newbie in go to be sure on this.

Thanks, Nicolas

maxcnunes commented 5 years ago

hey @nsteinmetz, sorry for the delay, I was quite busy lately. That commit I just pushed should fix it.

nsteinmetz commented 5 years ago

@maxcnunes no problem, it happens.

Looking at the commit, I spotted the right piece of code - not that bad :-)

Will test it soon :)

Thanks !

nsteinmetz commented 5 years ago

Cool, it works - thanks !