Refactors testnetify and remove last localOsmosis Dockerfile #2795

niccoloraspa · 2022-09-20T11:08:50Z

What is the purpose of the change

As per the title, this PR:

refactors the testnetify scripts
removes the localOsmosis Dockerfile

Main changes:

Give code a better structure
Create a CLI tool with multiple arguments
Decouple the testnetify process from the osmosisd binary. All the needed addresses are now expected as input.
Reuse the main Dockerfile to avoid creating a dedicated one
Significant speedup was obtained by:
- reading/writing from disk only at the beginning/end
- Replace the sed_inplace with the replace function that works in-memory
- IBC module pruning
Run the testnetify.py at docker run time and not at docker build time

Brief Changelog

Refactor testnetify scripts
Remove the localOsmosis Dockerfile

Testing and Verifying

I have updated the documentation that can be followed to test the process.

Main parts:

Copy state_export.json in tests/localosmosis/state_export/state_export.json
localnet-build-state-export
localnet-start-state-export

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? no
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
How is the feature or change documented? not applicable

niccoloraspa · 2022-09-20T11:13:38Z

A good way to review changes and understand the new flow is to go to top-down:

Inspect docker-compose.yaml
Inspect start.sh
Inspect testnetify.py

czarcas7ic

Awesome job with this!! Its really awesome to see something that is actually understandable compared to the previous testnetify script. Would like to discuss some of these points and then do another review before merging if possible!

tests/localosmosis/state_export/scripts/testnetify.py

czarcas7ic · 2022-09-20T17:05:25Z

Makefile

 localnet-remove-state-export:
-	@docker-compose -f tests/localosmosis/mainnet_state/docker-compose-state-export.yml down
+	rm -rf $(PWD)/tests/localosmosis/state_export/.osmosisd


Are we utilizing a .osmosisd folder within the repo itself to retain data? If so, we should change this to utilize the .osmosisd folder located in the home directory.

Yes, we're using a .osmosisd folder within the repo.
Same behavior of regular localOsmosis.

Should we change both?
TBH I prefer keeping it inside the repo but it's not a problem changing it.

Can we change this on both? From personal experience (today actually), I was confused where it was pulling the data from and had to remake a testnet instead of upgrading it. With the knowledge I have now I realize I could have done the following:

Move the .osmosisd repo to the home folder

Checkout v12

Move the .osmosisd repo back to the in repo location

This seems like a hassle when we could just keep it using .osmosisd. What are your thoughts though? Totally down to keep it this way if there is a good reason!

The osmosis home makes more sense, to be honest.
Using the .osmosisd folder within the repo was done for simplicity.

The initialization step of both localOsmosis setups (modifying the various tomls and genesis) runs only if no .osmosisd/config folder it's found: https://github.com/osmosis-labs/osmosis/blob/main/tests/localosmosis/scripts/setup.sh#L87

The check allows the possibility to stop/restart and restart the chain without repeating the initialization step as well as persisting the data.

If we want to use the .osmosisd folder in the home, we need to handle the first run of the script ensuring that the ~/.osmosisd folder is empty otherwise the init phase won't run

tests/localosmosis/state_export/scripts/start.sh

niccoloraspa · 2022-09-20T18:47:36Z

Implemented some changes and addressed the other points

czarcas7ic · 2022-09-21T00:59:26Z

Side note, does args.verbose default to True? If not I think it should imo.

Only question remaining is discussion on .osmosisd folder, other than that everything else looks good to go!

niccoloraspa · 2022-09-21T12:20:41Z

Side note, does args.verbose default to True? If not I think it should imo.

Only question remaining is discussion on .osmosisd folder, other than that everything else looks good to go!

verbose defaults to false

czarcas7ic · 2022-09-21T14:09:39Z

@niccoloraspa do you agree that verbose should default to true? Also, would you like to just merge this as is and address the .osmosisd issue in a subsequent PR or do you want to do this in this PR? I'll approve now so you can decide whatever works best for you!

niccoloraspa · 2022-09-22T08:00:57Z

@niccoloraspa do you agree that verbose should default to true? Also, would you like to just merge this as is and address the .osmosisd issue in a subsequent PR or do you want to do this in this PR? I'll approve now so you can decide whatever works best for you!

No strong preference, I'm happy with verbose output as a default.
I will invert the logic and rename the -v, --verbose flag to -q, --quiet to make the output less verbose if needed.

czarcas7ic · 2022-09-22T17:24:15Z

As discussed, merging this, then fixing/merging #2813. From there we can make a fix for utilizing the home folder

* Refactor testnetify and remove Dockerfile * Manually replace new validator public key * Improve output formatting * Bug fixes * Add ACCOUNT_PUBKEY and ACCOUNT_ADDRESS input * Add environment variables as input and config folder check * Update docs, Makefile and rename folder * Modify CLI arguments * Update docker-compose.yaml defaults and print updated values * Invert printing logic replacing verbose with quiet (cherry picked from commit 3fa5450) # Conflicts: # tests/localosmosis/README.md

…#2795) (#2822) * Refactors testnetify and remove last localOsmosis Dockerfile (#2795) * Refactor testnetify and remove Dockerfile * Manually replace new validator public key * Improve output formatting * Bug fixes * Add ACCOUNT_PUBKEY and ACCOUNT_ADDRESS input * Add environment variables as input and config folder check * Update docs, Makefile and rename folder * Modify CLI arguments * Update docker-compose.yaml defaults and print updated values * Invert printing logic replacing verbose with quiet (cherry picked from commit 3fa5450) # Conflicts: # tests/localosmosis/README.md * fix md conflicts Co-authored-by: Niccolo Raspa <[email protected]> Co-authored-by: Adam Tucker <[email protected]>

Niccolo Raspa added 7 commits September 20, 2022 07:24

Refactor testnetify and remove Dockerfile

2c426d5

Manually replace new validator public key

3084dbb

Improve output formatting

bc4aa28

Bug fixes

be3b6fc

Add ACCOUNT_PUBKEY and ACCOUNT_ADDRESS input

5c518b2

Add environment variables as input and config folder check

953ab8a

Update docs, Makefile and rename folder

439a347

niccoloraspa requested a review from a team September 20, 2022 11:08

github-actions bot added C:docs Improvements or additions to documentation T:build labels Sep 20, 2022

ValarDragon requested a review from czarcas7ic September 20, 2022 13:56

czarcas7ic requested changes Sep 20, 2022

View reviewed changes

Niccolo Raspa added 2 commits September 20, 2022 20:33

Modify CLI arguments

1892918

Update docker-compose.yaml defaults and print updated values

9226883

czarcas7ic approved these changes Sep 21, 2022

View reviewed changes

Invert printing logic replacing verbose with quiet

801f156

czarcas7ic merged commit 3fa5450 into main Sep 22, 2022

czarcas7ic deleted the feat/refactor-testnetify branch September 22, 2022 17:24

czarcas7ic added the A:backport/v12.x backport patches to v12.x branch label Sep 22, 2022

mergify bot mentioned this pull request Sep 22, 2022

Refactors testnetify and remove last localOsmosis Dockerfile (backport #2795) #2822

Merged

czarcas7ic added the A:backport/v11.x backport patches to v11.x branch label Sep 22, 2022

mergify bot mentioned this pull request Sep 22, 2022

Refactors testnetify and remove last localOsmosis Dockerfile (backport #2795) #2823

Closed

niccoloraspa mentioned this pull request Sep 22, 2022

DOCKER_BUILDKIT=1 is not passed when using docker-compose #2818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors testnetify and remove last localOsmosis Dockerfile #2795

Refactors testnetify and remove last localOsmosis Dockerfile #2795

niccoloraspa commented Sep 20, 2022 •

edited

Loading

niccoloraspa commented Sep 20, 2022 •

edited

Loading

czarcas7ic left a comment

czarcas7ic Sep 20, 2022 •

edited by niccoloraspa

Loading

niccoloraspa Sep 20, 2022 •

edited

Loading

czarcas7ic Sep 21, 2022

niccoloraspa Sep 21, 2022 •

edited

Loading

niccoloraspa commented Sep 20, 2022

czarcas7ic commented Sep 21, 2022

niccoloraspa commented Sep 21, 2022

czarcas7ic commented Sep 21, 2022

niccoloraspa commented Sep 22, 2022

czarcas7ic commented Sep 22, 2022

Refactors testnetify and remove last localOsmosis Dockerfile #2795

Refactors testnetify and remove last localOsmosis Dockerfile #2795

Conversation

niccoloraspa commented Sep 20, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

niccoloraspa commented Sep 20, 2022 • edited Loading

czarcas7ic left a comment

Choose a reason for hiding this comment

czarcas7ic Sep 20, 2022 • edited by niccoloraspa Loading

Choose a reason for hiding this comment

niccoloraspa Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

czarcas7ic Sep 21, 2022

Choose a reason for hiding this comment

niccoloraspa Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

niccoloraspa commented Sep 20, 2022

czarcas7ic commented Sep 21, 2022

niccoloraspa commented Sep 21, 2022

czarcas7ic commented Sep 21, 2022

niccoloraspa commented Sep 22, 2022

czarcas7ic commented Sep 22, 2022

niccoloraspa commented Sep 20, 2022 •

edited

Loading

niccoloraspa commented Sep 20, 2022 •

edited

Loading

czarcas7ic Sep 20, 2022 •

edited by niccoloraspa

Loading

niccoloraspa Sep 20, 2022 •

edited

Loading

niccoloraspa Sep 21, 2022 •

edited

Loading