fix(dgraph): Fixes misleading error about invalid hostname

[ahsanbarkati]

Fixes DGRAPH-1315

The issue was that when starting alpha with --my option and the port is not specified, or the port is invalid, or in general if there is any issue with the format of hostname:port then the same error was logged as "hostname is not valid"

This PR improves this error logs by logging the exact reason.

---

This change is 

Docs Preview:

[CLAassistant]

All committers have signed the CLA.

[ashish-goswami]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @ahsanbarkati, @manishrjain, @martinmr, and @vvbalaji-dgraph)

---

worker/groups.go, line 86 at r1 (raw file):

		if err := x.ValidateAddress(x.WorkerConfig.MyAddr); err != nil {
			log.Fatalf("%v", err)
		}

---

x/x.go, line 594 at r1 (raw file):

Quoted 5 lines of code…

	if !regExpHostName.MatchString(host) {
		return fmt.Errorf("hostname: %s is not valid", host)
	}

    return nil

---

x/x_test.go, line 81 at r1 (raw file):

			st := subtest
			t.Run(st.name, func(t *testing.T) {
				require.Equal(t, st.isValid, ValidateAddress(st.address) == nil)

Would be good to change test table and replace bool with error.

---

x/x_test.go, line 100 at r1 (raw file):

			st := subtest
			t.Run(st.name, func(t *testing.T) {
				require.Equal(t, st.isValid, ValidateAddress(st.address) == nil)

Same here.

[martinmr]

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ahsanbarkati, @ashish-goswami, @manishrjain, and @vvbalaji-dgraph)

---

go.sum, line 404 at r1 (raw file):

github.com/spf13/viper v1.3.2/go.mod h1:ZiWeW+zYFKm7srdB9IoDzzZXaJaI5eL9QjNiN/DMA2s=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.1.1 h1:2vfRuCMp5sSVIDSqO8oNnWJq7mPa6KVP3iPIwFBuy8A=

is this change needed?

---

worker/groups.go, line 86 at r1 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

		if err := x.ValidateAddress(x.WorkerConfig.MyAddr); err != nil {
			log.Fatalf("%v", err)
		}

you can also use x.Check

---

x/x.go, line 581 at r1 (raw file):

	}
	if p, err := strconv.Atoi(port); err != nil || p <= 0 || p >= 65536 {
		return fmt.Errorf("port: %d is not valid", p)

use errors.Errorf instead of fmt.Errorf to match the rest of the codebase.

---

x/x_test.go, line 78 at r1 (raw file):

			{"Invalid with port", "12.0.0:3333", true},
		}
		for _, subtest := range testData {

for _, st := range

then you can delete the line right below.

---

x/x_test.go, line 81 at r1 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

Would be good to change test table and replace bool with error.

Normally, I'd agree but we'd have to use an if statement so I think it's fine to keep it as is.

if st.isValid {
  require.NoError(t, ValidateAddress(st.address))
} else {
  require.Error(t, ValidateAddress(st.address))
} 

---

x/x_test.go, line 98 at r1 (raw file):

		for _, subtest := range testData {
			st := subtest

same comment here as above.

[parasssh]

Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on @ahsanbarkati, @ashish-goswami, @manishrjain, @martinmr, and @vvbalaji-dgraph)

[ahsanbarkati]

Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on @ashish-goswami, @manishrjain, @martinmr, and @vvbalaji-dgraph)

---

go.sum, line 404 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

is this change needed?

This is a patch change. Not sure if this is strictly required.

---

worker/groups.go, line 86 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

you can also use x.Check

Done.

---

x/x.go, line 581 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

use errors.Errorf instead of fmt.Errorf to match the rest of the codebase.

Done.

---

x/x.go, line 594 at r1 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

	if !regExpHostName.MatchString(host) {
		return fmt.Errorf("hostname: %s is not valid", host)
	}

    return nil

Done.

---

x/x_test.go, line 78 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

for _, st := range

then you can delete the line right below.

Done.

---

x/x_test.go, line 81 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Normally, I'd agree but we'd have to use an if statement so I think it's fine to keep it as is.

if st.isValid {
  require.NoError(t, ValidateAddress(st.address))
} else {
  require.Error(t, ValidateAddress(st.address))
} 

Is adding that extra if condition not worth considering our tests would be more rigorous then?

---

x/x_test.go, line 98 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

		for _, subtest := range testData {
			st := subtest

same comment here as above.

Done.

---

x/x_test.go, line 100 at r1 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

Same here.

Done.

[martinmr]

some comments about the tests but otherwise

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ahsanbarkati, @ashish-goswami, @manishrjain, @martinmr, and @vvbalaji-dgraph)

---

x/x.go, line 581 at r2 (raw file):

	}
	if p, err := strconv.Atoi(port); err != nil || p <= 0 || p >= 65536 {
		return errors.Errorf("Invalid port")

nit: print the invalid port in the error message

---

x/x.go, line 591 at r2 (raw file):

	}
	if !regExpHostName.MatchString(host) {
		return errors.Errorf("Invalid hostname")

nit: print the hostname as well

---

x/x_test.go, line 81 at r1 (raw file):

Previously, ahsanbarkati wrote…

Is adding that extra if condition not worth considering our tests would be more rigorous then?

I didn't say that. I said the refactoring is not necessary. Your code looks fine as is.

---

x/x_test.go, line 80 at r2 (raw file):

errors.Errorf("Invalid port")

this can be strings

---

x/x_test.go, line 85 at r2 (raw file):

			t.Run(st.name, func(t *testing.T) {
				if st.err != nil {
					require.Equal(t, st.err.Error(), ValidateAddress(st.address).Error())

You can use require.Contains as well once the error includes the address to save you from having to construct the entire error message.

require.Contains(t, st.err, ValidateAddress(st.address).Error())

This assumes st.err is a string.

---

x/x_test.go, line 87 at r2 (raw file):

					require.Equal(t, st.err.Error(), ValidateAddress(st.address).Error())
				} else {
					require.Nil(t, ValidateAddress(st.address))

require.NoError

---

x/x_test.go, line 100 at r2 (raw file):

		}{
			{"Valid without port", "[2001:db8::1]",
				errors.Errorf("address [2001:db8::1]: missing port in address")},

this errors can also be strings

[ahsanbarkati]

Reviewable status: 2 of 4 files reviewed, 11 unresolved discussions (waiting on @ahsanbarkati, @ashish-goswami, @manishrjain, @martinmr, and @vvbalaji-dgraph)

---

x/x_test.go, line 81 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I didn't say that. I said the refactoring is not necessary. Your code looks fine as is.

Done.

---

x/x_test.go, line 80 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

errors.Errorf("Invalid port")

this can be strings

Done.

---

x/x_test.go, line 85 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

You can use require.Contains as well once the error includes the address to save you from having to construct the entire error message.

require.Contains(t, st.err, ValidateAddress(st.address).Error())

This assumes st.err is a string.

I have included the ports and address in the expected strings.

---

x/x_test.go, line 87 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

require.NoError

Done.

---

x/x_test.go, line 100 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

this errors can also be strings

Done.