-
Notifications
You must be signed in to change notification settings - Fork 184
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
feat(cli): declarative deployment #1702
Conversation
🦋 Changeset detectedLatest commit: 339ffd1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
fc9f8c8
to
2e3d242
Compare
) | ||
); | ||
|
||
// TODO: move each system access+registration to batch call to be atomic |
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.
cc @CrazyNorman
there's a few other TODOs for batchCall
in here but I think this is one place we can add it that has meaningful impact to the deploy functionality vs. just making it faster
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.
cc @CrazyNorman
there's a few other TODOs for
batchCall
in here but I think this is one place we can add it that has meaningful impact to the deploy functionality vs. just making it faster
Got you. So If I'm getting you right, we can use batchCall
to do atomic setup, handling each system as a whole, one by one. This will make the deployment steps 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.
Exactly! If we for some reason fail to register a system but add access control, or vice versa, that feels like a weird state to be in. Using batchCall
here should help keep the operation atomic so if one step in setting up the system fails, the entire system setup fails.
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 very excited about this! Let's 🚢
This doesn't check namespace ownership/access yet and will throw an error or revert if we attempt an operation on a namespace we don't have access to. This could be improved by doing some checks during the "plan" step.This is now included.dev-contracts
CLI command is simplified to 1) watch files that would affect the world and 2) rebuild/redeploy everything. Since the deploy step is much smarter and only does the minimal work necessary, I am hoping the speed gains here can justify re-running build for now (to keep the logic simpler). Worth a refactor in the future when we have more time to spend on making this great.