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

Update instructions for running from source + docker #129

Merged
merged 10 commits into from
Jan 29, 2021

Conversation

eelanagaraj
Copy link
Contributor

@eelanagaraj eelanagaraj commented Dec 15, 2020

First pass at updating the README -- still needs some work, but most of the very out-of-date sections have been removed and the instructions for running from source/docker have been updated and should run as expected.

Copy link

@barbaraliau barbaraliau left a comment

Choose a reason for hiding this comment

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

Looks good! See comments.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@barbaraliau barbaraliau left a comment

Choose a reason for hiding this comment

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

submitting some comments while i go through the rest of the PR

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@barbaraliau barbaraliau left a comment

Choose a reason for hiding this comment

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

more comments

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@barbaraliau barbaraliau left a comment

Choose a reason for hiding this comment

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

I've tested almost everything and it looks like everything is g2g except I was getting build errors in the celo-monorepo for rc1. I'm going to assume that everything would work as expected though.

Actually, would this just be running on mainnet instead? Was the rc1 branch written before the launch of mainnet? If. so, then would any rc1 references need to be updated to mainnet/master?

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
* Run the Rosetta service in the background for the respective network (currently only alfajores)
* Run the CLI checks as follows:

```sh
# alfajores; specify construction or data
rosetta-cli check:construction --configuration-file PATH/TO/rosetta/rosetta-cli-conf/testnet/cli-config.json

Choose a reason for hiding this comment

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

I had to run ./bin/rosetta-cli check:construction ... to get this to run (from the directory I installed it), so recommending installing rosetta-cli into /usr/local/bin instead of where the command is run would be better. as well as adding instructions to update the command to point to the right path for the cli-config.json file

If on Mac, it is recommended to install rosetta-cli into /usr/local/bin

curl -sSfL https://raw.githubusercontent.com/coinbase/rosetta-cli/master/scripts/install.sh | sh -s -- -b /usr/local/bin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hesitant to add the actual install instructions for the cli here since that'll be an additional piece that we need to ensure is kept up to date (with the rosetta-cli README), so I'd prefer deferring to their installation instructions for that reason. That said, I'll add a note that installing to /usr/local/bin will allow you to use rosetta-cli directly on the command line (without the path to it)

* Run the Rosetta service in the background for the respective network (currently only alfajores)
* Run the CLI checks as follows:

```sh
# alfajores; specify construction or data
rosetta-cli check:construction --configuration-file PATH/TO/rosetta/rosetta-cli-conf/testnet/cli-config.json

Choose a reason for hiding this comment

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

I'm actually not sure if I can run checks becaus eI just get a bunch of these

2021/01/28 14:48:08 waiting for implementation to reach tip before testing...
[MEMORY] Heap: 1103.278084MB Stack: 0.687500MB System: 1260.756973MB GCs: 5
[STATS] Transactions Confirmed: 0 (Created: 0, In Progress: 0, Stale: 0, Failed: 0) Addresses Created: 0
2021/01/28 14:48:18 waiting for implementation to reach tip before testing...

How long does it normally take before it reaches the tip? Should I leave it running for a while? If so, I'd add something about that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're all good! It depends on whether you're starting with a fresh node or one with data, etc. Will add a note about that, but yes if this is the first time it'll take quite a while to reach the tip (syncing a full archive node). Data Checks themselves will take a long time since it needs to go through the entire chain + reconcile balances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ added a note about this

@barbaraliau barbaraliau self-requested a review January 28, 2021 21:52
Copy link

@barbaraliau barbaraliau left a comment

Choose a reason for hiding this comment

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

Oh but also either way I think these instructions can be merged with what is here and any other changes can be done in a different PR if you want. I'll leave it up to you to decide.

@eelanagaraj
Copy link
Contributor Author

I've tested almost everything and it looks like everything is g2g except I was getting build errors in the celo-monorepo for rc1. I'm going to assume that everything would work as expected though.

Actually, would this just be running on mainnet instead? Was the rc1 branch written before the launch of mainnet? If. so, then would any rc1 references need to be updated to mainnet/master?

@barbaraliau I believe mainnet == rc1. That said, for monorepo, this part is actually a bit more lax for now, as it should only affect the generated contract wrappers (and since there haven't been any relevant breaking changes, it should also even be fine to build a later version of master as long as it's pre-breaking changes). (This is something we'll have to address anyways when dealing with contract upgrades, but i'd suggest we punt it until that comes)

@eelanagaraj
Copy link
Contributor Author

@barbaraliau addressed your comments, I think it makes sense to push so it'll be easier to maintain + make sure it doesn't get out of date + need to be updated again. In the future, we can also add troubleshooting sections and such, esp. once we see what usage is like of the package. Thanks for your comments!

@eelanagaraj eelanagaraj merged commit f43c941 into master Jan 29, 2021
@eelanagaraj eelanagaraj deleted the eelanagaraj/update-readme branch January 29, 2021 12:32
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.

None yet

2 participants