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

Fix everything #7

Merged
merged 3 commits into from
Feb 23, 2016
Merged

Fix everything #7

merged 3 commits into from
Feb 23, 2016

Conversation

dignifiedquire
Copy link
Member

  • Rewrite generator code
  • Add source data to the repo (data)
  • Add tests for generator code
  • Refactor lookup code
  • Add tests for lookup code
  • Generate new data and add it to the 0.3 and 0.4 network (QmRn43NNNBEibc6m7zVNcS6UusB1u3qTTfyoLmkugbeeGJ)

Fixes #6

@dignifiedquire
Copy link
Member Author

Travis fails because it's not pinned on the 0.3 network probably and the resolves are too slow due to that


```bash
$ node geoip-gen.js path/GeoLite-Blocks.csv path/GeoLite-Location.csv
Copy link
Member

Choose a reason for hiding this comment

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

shall we add this script as a bin script to be installed and accessible globally? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's available as npm run generate as documented which I think is enough, but I'm okay adding it the bin scripts

@daviddias
Copy link
Member

image
woooooo. Do we really want to push the dataset to the repo?

@daviddias
Copy link
Member

Codewise LGTM :) great work @dignifiedquire !! Finally we have geoip working with proper tests and more docs :) 💃

@daviddias
Copy link
Member

One request, if all that travis needs is to have the dataset pinned on 0.3, can we do that to see if it goes green? It will be also useful for anyone else that might want to contribute as well.

@dignifiedquire
Copy link
Member Author

woooooo. Do we really want to push the dataset to the repo?

I don't know, I'd rather have it checked in than go search for it again in the future tbh

@dignifiedquire
Copy link
Member Author

One request, if all that travis needs is to have the dataset pinned on 0.3, can we do that to see if it goes green? It will be also useful for anyone else that might want to contribute as well.

I want to make it green, but pinbot keeps dying when trying to pin it :(

@ghost
Copy link

ghost commented Feb 18, 2016

I don't know, I'd rather have it checked in than go search for it again in the future tbh

Let's put it into IPFS and pin.

This dataset: ipfs.io/ipfs/<hash1>
produces this merkledag: ipfs.io/ipfs/<hash2>
Latest dataset available here: someurl.com/dataset.tgz

@jbenet
Copy link

jbenet commented Feb 21, 2016

Yes, put it into IPFS, not this repo. We should pin it.

(We should look into making a pinbot that works with github repos.)

@dignifiedquire
Copy link
Member Author

@lgierth @jbenet done, all data is now on ipfs, and gets downloaded automatically when generate is run :)

@ghost
Copy link

ghost commented Feb 21, 2016

Great 👍 could you squash the commits so that we don't bloat the repo size unneccessarily?

@dignifiedquire
Copy link
Member Author

@lgierth will do. any updates on pinning location data on 0.3? pinbot always dies

@@ -1,260 +0,0 @@
name,alpha2,countryCallingCodes,alpha3,ioc,currencies,languages,ccTLD,status
Copy link

Choose a reason for hiding this comment

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

where did this file go? is it not needed anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jbenet onto ipfs

@jbenet
Copy link

jbenet commented Feb 22, 2016

@dignifiedquire would be nice to make CI pass the tests, that way merging changes is way easier / safer. fine to be a new issue + separate PR.

@dignifiedquire
Copy link
Member Author

@jbenet they will pass when 0.3 has the geo data

@dignifiedquire
Copy link
Member Author

Merging this for now, tests will work as soon as the 0.3 issues are resolved. But I need a new release for use in ipfs/webui.

dignifiedquire added a commit that referenced this pull request Feb 23, 2016
@dignifiedquire dignifiedquire merged commit 139288c into master Feb 23, 2016
@dignifiedquire dignifiedquire deleted the fix branch February 23, 2016 10:48
@daviddias
Copy link
Member

What is blocking us from pinning it on 0.3? Is there an issue in infrastructure to track that progress that can be linked from here?

@ghost
Copy link

ghost commented Feb 23, 2016

What is blocking us from pinning it on 0.3? Is there an issue in infrastructure to track that progress that can be linked from here?

nothing -- I'm on it

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.

3 participants