-
Notifications
You must be signed in to change notification settings - Fork 6
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
build and tests proposals by stages #4
Conversation
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.
Looking good so far!
Makefile
Outdated
agoric-upgrade-8-1: agoric-upgrade-8 | ||
$(BUILD) --target agoric-upgrade-8-1 -t $(REPOSITORY):agoric-upgrade-8-1$(TAG_SUFFIX) | ||
|
||
agoric-upgrade-9: agoric-upgrade-8-1 | ||
agoric-upgrade-9: agoric-upgrade-8 |
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 I mentioned in chat, 8-1 is also known as PismoB. It existed to run a few blocks using that version of agd to verify that it wasn't horribly broken. I'm okay with you explicitly deciding to skip it, but it makes me wonder what criteria are used to decide which upgrades are important and which are not.
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.
This looks good to me so far. Does the current flow surface any breaking changes -- If I change the templates, and they result in an invalid Dockerfile, would the existing CI error in an obvious way?
Reviewers, I think this is close to landable. I still have to clean up the commit history and some docs (DESIGN.md) but it's ready for review. I just noticed that the last CI failed but I know it had passed for me locally so I don't expect that fixing it will change much about the code. Please review for ergonomics in addition to correctness. Try checking it out and running the paces. I don't expect to incorporate all ergonomics feedback into this PR but I will triage and queue up the additional work. We're trying to get this in soon to make #3 easier to land. |
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.
Looks like useful progress.
I have not yet run it locally.
-f Dockerfile upgrade-test-scripts | ||
- name: run final proposal | ||
run: docker run --env "DEST=0" docker-upgrade-test:latest | ||
- run: yarn global add tsx |
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 struggled to read the diff here... I guess name:
isn't required?
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.
right, it defaults to the run
string
@@ -0,0 +1,5 @@ | |||
{ |
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 don't see a .prettierrc.json
in https://github.com/Agoric/agoric-sdk/tree/master . Why do it differently 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.
sdk's prettier config is in package.json
. I thought it would be confusing to have a package.json
here because there's no JS at the top level
|
||
```shell | ||
make build |
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.
Did "how to use it" move somewhere else? I don't see it.
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's in the TODO now.
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.
ah. very well.
I did look in the TODO list without seeing it, but as long as that's the intent, cool.
### Troubleshooting | ||
If you get an error about port 26656 already in use, you have a local chain running on your OS. |
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.
The troubleshooting info seems useful.
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, I just wasn't sure which parts were accurate anymore so figured no docs are better than wrong docs. I have a TODO to increase (accurate) documentation. I hope that's okay to do in follow-up PRs
proposals/package.json
Outdated
"dependencies": { | ||
"ava": "^5.3.1", | ||
"better-sqlite3": "^8.5.1", | ||
"execa": "^7.2.0", |
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.
we seem to declare these dependencies a number of times. is this by design?
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.
tech debt at this point. I'll see if I can get rid of this package.json in this PR
// 'rm' to remove the container when it exits | ||
const cmd = `docker run --rm ${name}`; |
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.
The Makefile
had several other args to docker run
. We no longer need them?
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.
they were only conveniences and they don't work anymore.
I will develop new conveniences but I don't see those as blockers for this PR
await voteLatestProposalAndWait(); | ||
}; | ||
|
||
export const runZcfUpgrade = async (zcfBundleId, zoeBundleId) => { |
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.
All this ZCF stuff is gone on purpose?
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.
upgrade 12 is in agoric-sdk, not this repo until the upgrade passes. per the discussion with @mhofman
to be handled elsewhere
f7e488b
to
6bc407a
Compare
Part of Agoric/agoric-sdk#8376
On the way to Agoric/agoric-sdk#8317
This makes the Dockerfile dynamically from the proposal sequence declarations. It also introduces the ability to test core-eval proposals in their own layer.