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 writeLocalhostDeployment to deploy command #816

Conversation

SebastienGllmt
Copy link
Contributor

This is an alternative to #809 to also solve #791

Copy link
Contributor

@zoeyTM zoeyTM left a comment

Choose a reason for hiding this comment

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

This looks good overall! I did add one small suggestion for the wording of the flag description. Also, can you add a unit test for this case as well. Once those two things are taken care of this looks good to merge 👍

@@ -93,6 +93,10 @@ ignitionScope
.addOptionalParam("strategy", "Set the deployment strategy to use", "basic")
.addFlag("reset", "Wipes the existing deployment state before deploying")
.addFlag("verify", "Verify the deployment on Etherscan")
.addFlag(
"writeLocalhostDeployment",
"Write deployment information to disk even when running for in-memory networks"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Write deployment information to disk even when running for in-memory networks"
"Write deployment information to disk when deploying to the in-memory network"

Copy link
Contributor

@zoeyTM zoeyTM left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the PR!

@zoeyTM zoeyTM merged commit f77041e into NomicFoundation:development Sep 25, 2024
6 checks passed
@SebastienGllmt
Copy link
Contributor Author

Thanks for merging! Excited for this to be included in the next release so we can use it easily!

@zoeyTM zoeyTM mentioned this pull request Sep 26, 2024
@kanej kanej mentioned this pull request Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants