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

Several improvements to deploy subnet script #874

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fridrik01
Copy link
Contributor

@fridrik01 fridrik01 commented Apr 10, 2024

  • Fix relayer not starting due to missing argument
  • Fix bug when default-key was first in the json array
  • Streamline local deployment by introducing a prepare_local.sh script
  • Improve documentation for local deployment and document more known issues
  • Fix Mac support

Closes ENG-849
Closes ENG-857

@fridrik01 fridrik01 force-pushed the improve-deployment-script branch 3 times, most recently from 4f3efdc to 5e1791d Compare April 10, 2024 10:48
@fridrik01 fridrik01 requested a review from a team April 10, 2024 13:35
@fridrik01 fridrik01 marked this pull request as ready for review April 10, 2024 13:36
Copy link

linear bot commented Apr 10, 2024

ENG-849 Fix issues with deploy subnet script

Comment on lines 21 to 27
echo "Removing existing .ipc folder"
rm -rf ~/.ipc
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit brutal. Should it perhaps make a backup, if it doesn't exist yet?

Copy link
Contributor

@aakoshh aakoshh Apr 11, 2024

Choose a reason for hiding this comment

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

Just wondering: if you have this nice toml CLI, could it not upsert the setting of the specific subnet which is /r314159, rather than deleting everything the user had potentially configured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems a bit brutal. Should it perhaps make a backup, if it doesn't exist yet?

Maybe call it reset_local_.sh and/or add a prompt to confirm to user that it will reset your ipc folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

The upsert trick from @aakoshh would work for the ipc-cli, but this folder is also storing validator config and keys. @fridrik01 alternatively create a timestamped folder?

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

I am not familiar with these deployment scripts, I have to defer to someone else here.

@fridrik01 fridrik01 force-pushed the improve-deployment-script branch from 8f78f53 to fed218a Compare April 17, 2024 10:29
Copy link

linear bot commented Apr 18, 2024

@raulk raulk changed the title Several improvents to deploy subnet script Several improvements to deploy subnet script Apr 22, 2024
Copy link
Contributor

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Pinged @fridrik01 for action here.

How are we planning to move this PR forward? I think we've established that rm'ing the .ipc folder is undesirable. Can we rotate it instead? (I liked @aakoshh’s suggestion, but it only solves part of the problem.)

@fridrik01 fridrik01 force-pushed the improve-deployment-script branch from d06ecd9 to 9841ec3 Compare April 22, 2024 16:21
@fridrik01
Copy link
Contributor Author

@raulk I updated the PR so it renames the ipc folder (to include a timestamp) when the script runs so nothing gets deleted

@fridrik01 fridrik01 requested a review from raulk April 24, 2024 10:26
fridrik01 and others added 5 commits May 8, 2024 13:47
- Fix relayer not starting due to missing argument
- Fix bug when default-key was first in the json array
- Streamline local deployment by introducing a prepare_local.sh script
- Improve documentation for local deployment and document more known issues
Co-authored-by: Jorge M. Soares <547492+jsoares@users.noreply.github.com>
@fridrik01 fridrik01 force-pushed the improve-deployment-script branch from a6f7583 to 7646278 Compare May 8, 2024 13:47
@fridrik01 fridrik01 requested a review from a team May 8, 2024 13:58
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.

4 participants