Geo Package First Take by zhongshixi · Pull Request #2 · blockthrough/geo

zhongshixi · 2023-10-02T15:53:18Z

Change Set

first take on geo package, including all functionalities and checks

zhongshixi · 2023-10-05T14:30:36Z

Change Set

first take on geo package, including all functionalities , unit test and CI using dagger

zhehlovvalentyn · 2023-10-05T14:53:21Z

+		t.Fatal(fmt.Errorf("embedFS.Open: %w", err))
+	}
+
+	maximind, err := NewMaxMindReader(file)


should it be something like geo.NewMaxMindReader?

it should, i will add it

zhehlovvalentyn · 2023-10-05T15:00:20Z

+func (m *Reader) IsTestDB() bool {
+	desc, ok := m.Reader.Metadata().Description["en"]
+	if !ok {
+		return false
+	}
+
+	return strings.Contains(desc, "Test Database")
+}


why do we need to test if db is test DB?

oh, got it. We use it for testing, to parse specific IPs.

it is a sanity check for consumer , so they know if they are using test DB for production or not

When/How a test db could be initialized versus a regular db?

nit: imho, whoever is calling the reader should know if it's a test environment or not. There's no benefit for the reader to know that information itself.

zhehlovvalentyn · 2023-10-05T15:07:23Z

+var embedFS embed.FS
+
+const (
+	maxmindCountryTestDB string = "test/MaxMind-DB/test-data/GeoIP2-Country-Test.mmdb" // test database, only covers NA, EU, AS continents


oh, this is just a link. We don't keep actual binary. Cool!

this is a submodule link, the repo still keeps the test db binary, however when you install the package, you are not going to get db binary as part of the golang library.

zhehlovvalentyn · 2023-10-05T15:09:22Z

+
+// CountryAlpha3Code - return 3-letter ISO code for country
+func (c Country) CountryAlpha3Code() string {
+	if c.isEmpty() || len(c.Country.IsoCode) != 2 {


we can create a new func
e.g.

func (c Country) isCountryAlpha2Code() bool { return c.isEmpty() || len(c.Country.IsoCode) != 2 }

nice! but i think the better name i think is isCountryAlpha2CodeValid()

diogogmt · 2023-10-06T17:29:23Z

+version: 2.1
+
+orbs:
+  codecov: codecov/codecov@3.2.5


Do we need the codecov and git orbs?

we do

codecov is a nice-to-have, i want the data to be reflected as badge

git is nice-to-have since it uses submodule, without git orb, i have to write my own submodule sync, just a simple helper

ManuelPoppeRichter · 2023-10-06T17:30:33Z

+	"github.com/oschwald/geoip2-golang"
+)
+
+// Country - a type definition on geoip2.Country data struct while providing heloer functions to retrieve certain data in convinent way and additional country data maxmind db does not provide


nit: providing heloer -> providing helper

diogogmt · 2023-10-06T17:31:03Z

+jobs:
+  unit-test:
+    docker:
+      - image: cimg/go:1.21.1


Where is the cimg/go coming from?
Can we switch to chainguard if we need to use a public image? https://edu.chainguard.dev/chainguard/chainguard-images/reference/go/

cimage is the circle ci's golang image

any specific reason you want to use chainguard image? any major benefit?

@diogogmt ok, this is interesting
https://edu.chainguard.dev/chainguard/chainguard-images/vuln-comparison/go/

i like this - however, there is a few things to consider

this image does not have built-in curl , i have to install it

i do not see how security is going to impact this package, we are not building an app container, it is just there to run dagger

cimage so far is the most convenient image so i do not have add extra steps to install tool chains to make things work

Fair enough. If it's the official image by CircleCI with some helper already installed sounds good to me 👍

diogogmt · 2023-10-06T17:31:19Z

+    resource_class: small
+    steps:
+      - checkout  
+      - git/checkout-with-submodules


Why with submodules?

as specified in presentation, without submodules, you can not use the test db file provided by MaxMind which is required in unit testing

https://github.com/maxmind/MaxMind-DB

I try to avoid copy-and-paste and if more feature is added to the repo, i can easily grab different test db for unit testing since i have a submodule for all of its test db

diogogmt · 2023-10-06T17:31:39Z

+          command: cd /usr/local && { curl -L https://dl.dagger.io/dagger/install.sh | sudo sh; cd -; }
+      - run:
+          name: unit test
+          command:  dagger run --progress plain go run ci/main.go --cov_file cov.out


diogogmt · 2023-10-06T17:32:09Z

@@ -0,0 +1,3 @@
+[submodule "test/MaxMind-DB"]


Why do we need a submodule?

#2 (comment)

diogogmt · 2023-10-06T17:39:31Z

@@ -1,2 +1,83 @@
-# geo
-provide a simple interface to provide geo-related data, support maxmind db only at the moment
+# Geo Information Library For Go


Well done with the README! 👍

diogogmt · 2023-10-06T17:40:16Z

+	"github.com/oschwald/geoip2-golang"
+)
+
+// Country - a type definition on geoip2.Country data struct while providing heloer functions to retrieve certain data in convinent way and additional country data maxmind db does not provide


suggestion: can you run the comments through a spell checker? eg; ~~heloer~~ -> helper

@diogogmt do you have a tool for this? my spell checker does not work on ReadMe

nvm, i found a vs code plugin, have run through a check, fixed the wording issue

diogogmt · 2023-10-06T17:40:47Z

+)
+
+// Country - a type definition on geoip2.Country data struct while providing heloer functions to retrieve certain data in convinent way and additional country data maxmind db does not provide
+// user is expected not to directly modify it.


If the user shouldn't modify it, why is it public instead of private?

because the design is - users still can access its internal data if they have more use case, and since the underlying structure is referred to a 3rd party package, it is not quite impossible to do so.

even you make it type country , you can still modify its internal, it is just user can not create a default country for their own

i will change it type country, since i do not expect user to create an empty Country struct

diogogmt · 2023-10-06T17:42:08Z

+}
+
+func NewMaxMind(b []byte) (*Reader, error) {
+	reader, err := geoip2.FromBytes(b)


This block of code overlaps with https://github.com/blockthrough/geo/pull/2/files#diff-d607e19697d2bc59fe4217b2de67b083970a90c4ac563ca72891bd13c4c052baR24

suggestion: reference a private function to read the bytes from geoip2.

diogogmt · 2023-10-06T17:43:38Z

+	return time.Unix(int64(m.Metadata().BuildEpoch), 0)
+}
+
+// Version - the opinionated semanic version for the underlying MaxMind DB in the format of v<major_version>.<minor_version>


~~semanic~~ -> semantic

Zhongshi Xi added 30 commits October 2, 2023 11:52

set up circle ci template

4b9a2eb

set up go mod

756d586

integrate with dagger

b887032

use dagger

e740b59

experimenting otherways of invoking dagger

baa3e50

fix circle ci

28e9e4d

fix circle ci

a84a709

fix circle ci

555d27b

fix circle ci

e6f445e

add ci

a075e1a

fix version of orbs

f11fbff

check daemon

8b1814e

docker machine

4d2fa57

use simple config

8ffc7e9

refactor code

3641924

refactor code

a80beb6

add more unit test

3baaba7

add badge and code coverage

bf0ff0c

add submodule

a81d399

add submodule

e756a78

sync submodule

cf6cde3

add git orbs

8fcac00

try fix orbs

fdde9fd

fix circle ci

ad69bba

circle ci

b33602a

test

2237d2e

add circle ci

bfc6a44

submodule

1854f29

refactor README

e53284f

use dagger as an example

64f8b5d

add comment

a33f2ec

add code cov

357b100

zhehlovvalentyn reviewed Oct 5, 2023

View reviewed changes

Zhongshi Xi added 2 commits October 5, 2023 12:53

add change according to the review

1b7d6c8

add flag

72de71c

diogogmt reviewed Oct 6, 2023

View reviewed changes

ManuelPoppeRichter reviewed Oct 6, 2023

View reviewed changes

diogogmt reviewed Oct 6, 2023

View reviewed changes

ManuelPoppeRichter approved these changes Oct 6, 2023

View reviewed changes

diogogmt reviewed Oct 6, 2023

View reviewed changes

fix documentation

dd6a3fc

diogogmt reviewed Oct 6, 2023

View reviewed changes

Zhongshi Xi added 5 commits October 6, 2023 13:46

make country private

2c23055

fix works using spell checker

0d19e8e

switch to chainguard

f241226

try alpine

351d148

switch back to cimg

37ddb53

diogogmt approved these changes Oct 6, 2023

View reviewed changes

lsmoura approved these changes Oct 6, 2023

View reviewed changes

zhongshixi merged commit f910162 into master Oct 10, 2023

Conversation

zhongshixi commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhongshixi commented Oct 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongshixi Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongshixi Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongshixi Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongshixi Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongshixi Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongshixi Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongshixi commented Oct 2, 2023 •

edited

Loading

zhongshixi Oct 6, 2023 •

edited

Loading

zhongshixi Oct 6, 2023 •

edited

Loading

zhongshixi Oct 6, 2023 •

edited

Loading

zhongshixi Oct 6, 2023 •

edited

Loading

zhongshixi Oct 6, 2023 •

edited

Loading

zhongshixi Oct 6, 2023 •

edited

Loading