geolocate: first pass of code review and minor fixes (#359)

* doc(geolocate): minor cleanup

* more minor cleanups of geolocate

* remove disabled test and see whether now it works

internal/engine/geolocate/README.md

@@ -1,3 +0,0 @@
-# Package github.com/ooni/probe-engine/geolocate
-
-Package geolocate implements IP lookup, resolver lookup, and geolocation.

internal/engine/geolocate/geolocate.go

@@ -5,21 +5,19 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/ooni/probe-cli/v3/internal/engine/model"
 	"github.com/ooni/probe-cli/v3/internal/engine/netx"
-	"github.com/ooni/probe-cli/v3/internal/runtimex"
 	"github.com/ooni/probe-cli/v3/internal/version"
 )
 
 const (
-	// DefaultProbeASN is the default probe ASN as number.
+	// DefaultProbeASN is the default probe ASN as a number.
 	DefaultProbeASN uint = 0
 
 	// DefaultProbeCC is the default probe CC.
 	DefaultProbeCC = "ZZ"
 
 	// DefaultProbeIP is the default probe IP.
-	DefaultProbeIP = model.DefaultProbeIP
+	DefaultProbeIP = "127.0.0.1"
 
 	// DefaultProbeNetworkName is the default probe network name.
 	DefaultProbeNetworkName = ""
@@ -46,40 +44,37 @@ var (
 type Logger interface {
 	Debug(msg string)
 	Debugf(format string, v ...interface{})
-	Info(msg string)
 	Infof(format string, v ...interface{})
-	Warn(msg string)
-	Warnf(format string, v ...interface{})
 }
 
-// Results contains geolocate results
+// Results contains geolocate results.
 type Results struct {
-	// ASN is the autonomous system number
+	// ASN is the autonomous system number.
 	ASN uint
 
-	// CountryCode is the country code
+	// CountryCode is the country code.
 	CountryCode string
 
 	// didResolverLookup indicates whether we did a resolver lookup.
 	didResolverLookup bool
 
-	// NetworkName is the network name
+	// NetworkName is the network name.
 	NetworkName string
 
-	// IP is the probe IP
+	// IP is the probe IP.
 	ProbeIP string
 
-	// ResolverASN is the resolver ASN
+	// ResolverASN is the resolver ASN.
 	ResolverASN uint
 
-	// ResolverIP is the resolver IP
+	// ResolverIP is the resolver IP.
 	ResolverIP string
 
-	// ResolverNetworkName is the resolver network name
+	// ResolverNetworkName is the resolver network name.
 	ResolverNetworkName string
 }
 
-// ASNString returns the ASN as a string
+// ASNString returns the ASN as a string.
 func (r *Results) ASNString() string {
 	return fmt.Sprintf("AS%d", r.ASN)
 }
@@ -123,16 +118,19 @@ type Config struct {
 	UserAgent string
 }
 
-// Must ensures that NewTask is successful.
+// discardLogger just ignores log messages thrown at it.
-func Must(task *Task, err error) *Task {
+type discardLogger struct{}
-	runtimex.PanicOnError(err, "NewTask failed")
+
-	return task
+func (*discardLogger) Debug(msg string) {}
-}
+
+func (*discardLogger) Debugf(format string, v ...interface{}) {}
+
+func (*discardLogger) Infof(format string, v ...interface{}) {}
 
 // NewTask creates a new instance of Task from config.
-func NewTask(config Config) (*Task, error) {
+func NewTask(config Config) *Task {
 	if config.Logger == nil {
-		config.Logger = model.DiscardLogger
+		config.Logger = &discardLogger{}
 	}
 	if config.UserAgent == "" {
 		config.UserAgent = fmt.Sprintf("ooniprobe-engine/%s", version.Version)
@@ -147,7 +145,7 @@ func NewTask(config Config) (*Task, error) {
 		probeASNLookupper:    mmdbLookupper{},
 		resolverASNLookupper: mmdbLookupper{},
 		resolverIPLookupper:  resolverLookupClient{},
-	}, nil
+	}
 }
 
 // Task performs a geolocation. You must create a new

internal/engine/geolocate/geolocate_test.go

@@ -265,7 +265,7 @@ func TestLocationLookupSuccessWithResolverLookup(t *testing.T) {
 
 func TestSmoke(t *testing.T) {
 	config := Config{}
-	task := Must(NewTask(config))
+	task := NewTask(config)
 	result, err := task.Run(context.Background())
 	if err != nil {
 		t.Fatal(err)

internal/engine/geolocate/ipconfig_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"net"
 	"net/http"
-	"os"
 	"testing"
 
 	"github.com/apex/log"
@@ -12,11 +11,6 @@ import (
 )
 
 func TestIPLookupWorksUsingIPConfig(t *testing.T) {
-	if os.Getenv("CI") == "true" {
-		// See https://github.com/ooni/probe-cli/pull/259/checks?check_run_id=2166066881#step:5:123
-		// as well as https://github.com/ooni/probe/issues/1418.
-		t.Skip("This test does not work with GitHub Actions")
-	}
 	ip, err := ipConfigIPLookup(
 		context.Background(),
 		http.DefaultClient,

internal/engine/geolocate/mmdblookup.go

@@ -28,7 +28,7 @@ func (mmdbLookupper) LookupASN(ip string) (asn uint, org string, err error) {
 }
 
 // LookupASN returns the ASN and the organization associated with the
-// given ip using the ASN database at path.
+// given IP address.
 func LookupASN(ip string) (asn uint, org string, err error) {
 	return (mmdbLookupper{}).LookupASN(ip)
 }

internal/engine/model/model.go

@@ -1,7 +1,9 @@
 // Package model defines shared data structures and interfaces.
 package model
 
+import "github.com/ooni/probe-cli/v3/internal/engine/geolocate"
+
 const (
 	// DefaultProbeIP is the default probe IP.
-	DefaultProbeIP = "127.0.0.1"
+	DefaultProbeIP = geolocate.DefaultProbeIP
 )

internal/engine/session.go

@@ -645,11 +645,11 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error {
 // LookupLocationContext performs a location lookup. If you want memoisation
 // of the results, you should use MaybeLookupLocationContext.
 func (s *Session) LookupLocationContext(ctx context.Context) (*geolocate.Results, error) {
-	task := geolocate.Must(geolocate.NewTask(geolocate.Config{
+	task := geolocate.NewTask(geolocate.Config{
 		Logger:    s.Logger(),
 		Resolver:  s.resolver,
 		UserAgent: s.UserAgent(),
-	}))
+	})
 	return task.Run(ctx)
 }