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

build and tests proposals by stages #4

Merged
merged 36 commits into from
Nov 2, 2023
Merged

build and tests proposals by stages #4

merged 36 commits into from
Nov 2, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Oct 26, 2023

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.

Copy link
Member

@michaelfig michaelfig left a 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
Comment on lines 16 to 19
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
Copy link
Member

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.

makeDockerfile.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@raphdev raphdev 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 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?

@turadg
Copy link
Member Author

turadg commented Nov 2, 2023

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.

Copy link
Member

@dckc dckc left a 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
Copy link
Member

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?

Copy link
Member Author

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 @@
{
Copy link
Member

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:/Agoric/agoric-sdk/tree/master . Why do it differently here?

Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Comment on lines -45 to -46
### Troubleshooting
If you get an error about port 26656 already in use, you have a local chain running on your OS.
Copy link
Member

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.

Copy link
Member Author

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

README.md Outdated Show resolved Hide resolved
proposals/55:statom-vaults/package.json Show resolved Hide resolved
"dependencies": {
"ava": "^5.3.1",
"better-sqlite3": "^8.5.1",
"execa": "^7.2.0",
Copy link
Member

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?

Copy link
Member Author

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

Comment on lines +10 to +25
// 'rm' to remove the container when it exits
const cmd = `docker run --rm ${name}`;
Copy link
Member

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?

Copy link
Member Author

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) => {
Copy link
Member

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?

Copy link
Member Author

@turadg turadg Nov 2, 2023

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

upgrade-test-scripts/lib/assert.js Show resolved Hide resolved
@turadg turadg enabled auto-merge November 2, 2023 22:32
@turadg turadg changed the title build Dockerfile by proposal sequence build and tests proposals by stages Nov 2, 2023
@turadg turadg merged commit a949621 into main Nov 2, 2023
1 check passed
@turadg turadg deleted the ta/docker-template branch November 2, 2023 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants