-
Notifications
You must be signed in to change notification settings - Fork 10
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
Switch to using eos-system-contracts
in libtester tests
#1178
Conversation
This reverts commit 3fdc354.
@@ -3,7 +3,7 @@ | |||
"target":"4.1", | |||
"prerelease":true | |||
}, | |||
"referencecontracts":{ | |||
"eossystemcontracts":{ | |||
"ref":"main" |
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.
Any reason to not set it back to a release too?
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 feel that main
is better, but maybe I'm not understanding the process well (my first time doing this)
For example I am working on some changes in eos-system-contracts
, and for the tests to pass I'll update defaults.json
to use my branch which has the changes. After my branch is merged to main, I think we'll have to switch defaults.json
back to main for the tests to continue passsing (assuming I have merged my eos-system-contracts
branch by then).
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 think the easiest way to imagine a problem with main
is if spring's release/1.2
is using main
, and spring's main
is using main
, and then system contract's main
changes in such a way that it requires spring's main
. Now it's impossible for any PRs to release/1.2
to pass since they'll still fetch contract's main
that won't work on release/1.2
.
So it's probably preferable to stick with something stable unless needing a branch for some reason, and at the very least when we prepare to branch a release branch we should try to pin it down.
Keep in mind too that reference contracts has no release branches, so that's a big reason it was left to languish as main
even though that's not ideal.
All that said, eos-system-contracts is using main
for spring and main
for cdt so 🤷♀️
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.
system contract's
main
changes in such a way that it requires spring'smain
That would require a hard fork, right?
But yes I see now that the previous release
branches of spring
using main
would have to be updated for the tests to pass. I guess the developer making a change to a release branch would see a ci/cd test failure, and would have to pin the branch for eos-system-contracts
for the tests to pass. Maybe that is the right time to pin the branch ref?
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.
No it does not require a hard fork. It could simply be a new interface that libtester provides that contracts tests begin to use.
Note:start |
Resolves #1177.
Now that we are not updating the
reference-contracts
repo anymore, switch the libtester tests to usingeos-system-contracts
.This reverts commit 3fdc354.