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

Update cell provisioning types #318

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

pdaoust
Copy link

@pdaoust pdaoust commented Jan 3, 2025

CellProvisioning broke compatibility at 0.4; there's now a protected flag for UseExisting. This adds that flag and also takes the opportunity to add CloneOnly which was previously missing.

I'd love some feedback on the TS idioms I've used here; I don't know how idiomatic or ergonomic they are.

Tests (including, but not only, the new ones I added) were failing locally due to API timeouts; hopefully that isn't masking any conductor errors (I believe I've fixed them all, but I'm not sure).

Closes #317 .

@pdaoust pdaoust requested a review from jost-s January 3, 2025 22:58
@@ -429,6 +427,12 @@ export type Location =
* Get file from URL
*/
url: string;
}
| {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This in particular feels weird to me -- an empty struct as a union variant. It's only there because Location gets intersected with AppRoleDnaManifest, because the corresponding stuff (that is, the optional AppRoleDnaManifest::location field's enum variant value) gets collapsed into AppRoleDnaManifest and we've got to find some similar way of modelling that.

@pdaoust pdaoust self-assigned this Jan 3, 2025
@pdaoust pdaoust marked this pull request as draft January 4, 2025 00:39
@pdaoust
Copy link
Author

pdaoust commented Jan 4, 2025

Tests were failing after the smoke cleared; converted to draft.

@pdaoust
Copy link
Author

pdaoust commented Jan 6, 2025

Currently getting timeouts with the installApp method with an app with at least one role that uses UseExisting or CloneOnly provisioning strategies. Happens on 0.4 too. I'm not getting anything terribly interesting from the log.

@pdaoust
Copy link
Author

pdaoust commented Jan 6, 2025

The CloneOnly strategy is blocked by an upstream bug holochain/holochain#4594 . The UseExisting test failure is a bit more rugose -- some sort of deserialisation error whereby it thinks I'm trying to pass a map of something as a cell ID, whereas I'm most certainly passing a CellId.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

CellProvisioning doesn't match the enum in holochain_types
1 participant