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

Add basic setup script #47

Merged
merged 5 commits into from
Sep 8, 2022
Merged

Add basic setup script #47

merged 5 commits into from
Sep 8, 2022

Conversation

hectorgomezv
Copy link
Member

@hectorgomezv hectorgomezv commented Sep 1, 2022

Closes #4

This is a very simple script to just start the Docker containers and create the superusers. As we discussed we can stay simple and let the user insert the ChainInfo. I updated the README.md file accordingly.

@hectorgomezv hectorgomezv requested a review from a team as a code owner September 1, 2022 16:15
@hectorgomezv hectorgomezv self-assigned this Sep 1, 2022
&& docker compose exec txs-web python manage.py createsuperuser \
|| exit

echo "==> $(date +%H:%M:%S) ==> All set! You may want to add a ChainInfo into the Config service. Please use the link below to fill its data: http://localhost:8000/cfg/admin/chains/chain/add/"
Copy link
Member

Choose a reason for hiding this comment

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

The third task of the issue is add ChainInfo, if you are using a default RPC for Polygon why don't add basic chainInfo for Polygon ❓

Copy link
Member

Choose a reason for hiding this comment

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

Also, when creating superusers, is expected for the user to input the password, right?

Copy link
Member Author

@hectorgomezv hectorgomezv Sep 2, 2022

Choose a reason for hiding this comment

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

The third task of the issue is add ChainInfo, if you are using a default RPC for Polygon why don't add basic chainInfo for Polygon ❓

If I recall correctly (because there was a while since 🙂 ) we concluded that maybe it's better to let the user enter the ChainInfo using the Django admin (context here). But I'm open to change it if you think we could make a REST call to retrieve the ChainInfo and inserting it (would require another PR to create the admin command in the Config Service) 🙂

Also, when creating superusers, is expected for the user to input the password, right?

Yes, the script execution waits until the user writes the desired username/password. Is this correct?

|| exit

echo "==> $(date +%H:%M:%S) ==> Using http://polygon-rpc.com as default RPC node"
sed -i '' 's|http://url.to.node|https://polygon-rpc.com|g' $TEMP \
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't do it here, I don't feel it very intuitive to tell the user to edit this file to get the node they want, and in the case we want it maybe set http://polygon-rpc.com inside a variable, like NODE_URL=http://polygon-rpc.com so you can reuse it also in the echo

Copy link
Member Author

@hectorgomezv hectorgomezv Sep 2, 2022

Choose a reason for hiding this comment

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

Yes, I agree, I saw this file like something the user wouldn't touch, but maybe it's a bit confusing, in the case they actually want to change the RPC url, to modify this file. What do you think it's a better option here?

  1. Setting a NODE_URL variable inside this file.
  2. Just including the default RPC url in the .env.sample file.
  3. Not setting a default at all, so the script will fail if the users don't set the RPC url themselves.
  4. Do you see another option?

🙂

Copy link
Member

Choose a reason for hiding this comment

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

I think users should configure .env as stated in the docs with the node they need and the rest of the configuration should be going on from there

Copy link
Member Author

@hectorgomezv hectorgomezv Sep 7, 2022

Choose a reason for hiding this comment

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

I've removed .env manipulation from the script in this commit. Now it just starts Docker containers and manages superuser creation 🙂

Copy link
Member

@Uxio0 Uxio0 left a comment

Choose a reason for hiding this comment

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

Nice!

@hectorgomezv hectorgomezv dismissed moisses89’s stale review September 8, 2022 07:08

Agreed on populating ChainInfo manually, please contact me if you think we need to do it programatically, I'll open another PR :)

@hectorgomezv hectorgomezv merged commit 20cb2a7 into main Sep 8, 2022
@hectorgomezv hectorgomezv deleted the add-basic-setup-script branch September 8, 2022 07:08
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.

Add convenience script for basic setup
3 participants