This repository has been archived by the owner on Apr 22, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improve
deploy.sh
#171Improve
deploy.sh
#171Changes from 10 commits
c0d50e6
22766d5
cc4677b
69483ca
db21ae0
75430ae
a104bf2
1e8e774
8c597b7
8581a61
a56a15a
b68131f
2114dc1
1b31068
af2f473
64687f3
35e6e9c
ae86e1b
c983f2b
ad6fcf0
e8afe75
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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)There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.