-
Notifications
You must be signed in to change notification settings - Fork 28
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
Improve deployment related code and scripts #896
Conversation
becedca
to
c833f94
Compare
printf "Do you want to proceed for $SEASON? Enter [mainnet]: " | ||
read CONFIRM | ||
printf "\n\n" | ||
if [[ ! $CONFIRM = "mainnet" ]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice~ 👍
else | ||
deploy_tracker "$NUM_PARENT_NODES" | ||
for j in `seq 0 $(( ${NUM_PARENT_NODES} - 1 ))` | ||
# Tracker server is deployed with BEGIN_PARENT_NODE_INDEX = -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
KEEP_DATA_OPTION="" | ||
FULL_SYNC_OPTION="" | ||
ACCOUNT_INJECTION_OPTION="" | ||
ACCOUNT_INJECTION_OPTION="--private-key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change this to just 'private-key'? It doensn't have the '--' prefix since the blockchain node param doesn't have it. I should've added some notes on the conversion..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. Just tried to export values properly keeping its values in the script like other variables. PTAL
printf "JSON_RPC_OPTION=$JSON_RPC_OPTION\n" | ||
printf "REST_FUNC_OPTION=$REST_FUNC_OPTION\n" | ||
|
||
# NOTE(liayoo): Currently this script supports [--keystore|--mnemonic] option only for the parent chain. | ||
if [[ $ACCOUNT_INJECTION_OPTION != "private_key" ]] && [[ "$SHARD_INDEX" -gt 0 ]]; then | ||
if [[ $ACCOUNT_INJECTION_OPTION != "--private_key" ]] && [[ "$SHARD_INDEX" -gt 0 ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
KEEP_DATA_OPTION="" | ||
FULL_SYNC_OPTION="" | ||
ACCOUNT_INJECTION_OPTION="" | ||
ACCOUNT_INJECTION_OPTION="--private-key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
# for performance test pipeline | ||
export ENABLE_EXPRESS_RATE_LIMIT=false | ||
# NOTE(liayoo): Currently this script supports [--keystore|--mnemonic] option only for the parent chain. | ||
if [[ $ACCOUNT_INJECTION_OPTION != "--private_key" ]] && [[ "$SHARD_INDEX" -gt 0 ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Addressed the comments so far, PTAL~ @liayoo |
@@ -100,18 +95,24 @@ if [[ "$ACCOUNT_INJECTION_OPTION" = "" ]]; then | |||
return 1 | |||
fi | |||
|
|||
if [[ $ACCOUNT_INJECTION_OPTION = "--keystore" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked! 👍
Should we test these scripts with different combinations of options? :) |
3686889
to
56d60d9
Compare
Thanks for asking. I tried to test it for as many combinations as I can. Basically, I think the most important combinations are |
…NODE_INDEX inclusive
e6e3963
to
f374ecc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the nice refactoring!!
Wow, is that like 64 combinations?! That's really impressive.. Thanks for testing so extensively! Ah, not actually all of them :) individually and most correlated cases (~ 10 cases). |
Thanks for the review! Now merging.. |
Change summary:
Related issues: