Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

Improve deploy.sh #171

Merged
merged 21 commits into from
Mar 16, 2023
Merged

Improve deploy.sh #171

merged 21 commits into from
Mar 16, 2023

Conversation

naszam
Copy link
Contributor

@naszam naszam commented Feb 20, 2023

Rationale

Following #170 pattern I've extended the deploy.sh script behaviour to automatically edit config.sol by replacing the address with the deployed one (works with both pre-existing address(0) or with deployed spell address address(0x123).

@naszam naszam marked this pull request as ready for review February 24, 2023 16:26
@naszam
Copy link
Contributor Author

naszam commented Feb 24, 2023

@brianmcmichael @gbalabasquer I think this PR could be reviewed and merged on master in the meantime, as not pending on #166 discussion.

@naszam
Copy link
Contributor Author

naszam commented Mar 1, 2023

Unified together with #170 to automate further the spell crafting process by relying on Etherscan APIs to retrieve transaction hash, including commit add-on feat to automate end-to-end following dapp-init pattern.

@naszam naszam requested a review from godsflaw March 1, 2023 17:31
Comment on lines 33 to 35
# commit edit change to config.sol
(set -x; git add src/test/config.sol)
(set -x; git commit -m "add deployed spell info")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should auto-commit this. First, I think this approach could be fragile (like one may have other staged changes for addition). Second, there is something that is a bit too sneaky about auto-commits without human review. Minimally, if we wanted to do this, I would say we should let this code run for a while and see if it encounters any failures under normal operation. But honestly, I think we may just want humans to be able to diff to review, add, and commit this manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this pattern is part of the spell crafting/reviewing automation endeavour, inspired by dapp init pattern.
In this specific case though, it could be a bit tricky as there are many moving pieces but one approach could be ensuring the changes have been actually edited in config.sol directly or better, running tests and if they pass commit.

Copy link
Contributor Author

@naszam naszam Mar 1, 2023

Choose a reason for hiding this comment

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

Added tests check at deployed block in 8581a61

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a fan of the auto-commit either. If this is part of an automation workflow, it may be better to have the auto-commit done in an external script that calls deploy.sh imo.

Copy link
Contributor Author

@naszam naszam Mar 3, 2023

Choose a reason for hiding this comment

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

As discussed in chat I'm testing some improvements to beef the script up with more checks before committing the changes as make test is a bit fragile an could return a zero status even if tests doesn't pass, but not opposed to handle the commit in an external script, I only think would be best for a CLI tool where the approach is more modular.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if a56a15a and b68131f give more assurance, the extra check was inspired by the dapp upgrade one (see here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussing with @olivdb in chat, I've addressed the request of adding a way to stash and then reload local staging area in 2114dc1 (note that the stash would happen before the script makes edits and then it's reloaded before commiting in case the dev exit the commit edit prompt)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after some testing and further discussions, using trap we ensure we always exit with restoring the staging area.

scripts/deploy.sh Outdated Show resolved Hide resolved
@naszam naszam requested a review from godsflaw March 1, 2023 20:34
godsflaw
godsflaw previously approved these changes Mar 2, 2023
@naszam naszam requested a review from olivdb March 2, 2023 17:16
olivdb
olivdb previously approved these changes Mar 3, 2023
@naszam naszam dismissed stale reviews from olivdb and godsflaw via a56a15a March 3, 2023 18:26
@naszam naszam requested review from godsflaw and olivdb March 4, 2023 20:32
scripts/deploy.sh Outdated Show resolved Hide resolved
scripts/deploy.sh Outdated Show resolved Hide resolved
@naszam naszam merged commit 642de7d into master Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants