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

feat(site-builder): add new package to config, expose destroy command #318

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

giac-mysten
Copy link
Collaborator

@giac-mysten giac-mysten commented Dec 5, 2024

Closes SEINT-357
Closes SEINT-371

@giac-mysten giac-mysten requested a review from Tzal3x December 5, 2024 10:44
Copy link

vercel bot commented Dec 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
walrus-sites-sp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2024 10:44am
walrus-sites-sp-testnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2024 10:44am
walrus-sites-sw ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2024 10:44am
walrus-sites-sw-testnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2024 10:44am

Copy link

codspeed-hq bot commented Dec 5, 2024

CodSpeed Performance Report

Merging #318 will not alter performance

Comparing gg/site-builder/update-package-and-burn (75c182f) with main (4d1e19e)

Summary

✅ 3 untouched benchmarks

Copy link
Collaborator

@Tzal3x Tzal3x left a comment

Choose a reason for hiding this comment

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

Sweet! Thank you 🙇‍♂️ Some newborn questions:

  1. This only deletes already "expired" sites right? Sites that the blob of the resources have been gone past their epoch expiration, so they are deleted. And the user just wants to take the reimbursement by deleting the no-longer needed sui objects.
  2. If a user needs to delete a site, before that expiring, this functionality is not supported yet in the site-builder right? Meaning that during publishing he should define the resource blobs as deleteable, delete them, and then have a separate function (maybe different than destroy?) that deletes the blobs first, and then calling site-builder destroy?

@VLegakis VLegakis self-requested a review December 5, 2024 12:42
@giac-mysten
Copy link
Collaborator Author

1. This only deletes already "expired" sites right? Sites that the blob of the resources have been gone past their epoch expiration, so they are deleted. And the user just wants to take the reimbursement by deleting the no-longer needed sui objects.
2. If a user needs to delete a site, before that expiring, this functionality is not supported yet in the `site-builder` right? Meaning that during publishing he should define the resource blobs as deleteable, delete them, and then have a separate function (maybe different than destroy?) that deletes the blobs first, and then calling `site-builder destroy`?

Ah this is actually an excellent point. Currently, the destroy command does not touch the blobs on walrus. Only removes the Site object and dynamic fields from Sui. Which may not be what people want. However, we do not support deletable sites at the moment. So a bit more thinking may be actually needed here to decide what is best to do.

Possibly a good idea would be to always publish sites as deletable blobs, unless someone explicitly adds the --permanent flag.

Copy link
Collaborator

@Tzal3x Tzal3x left a comment

Choose a reason for hiding this comment

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

Ok, let's merge this and we then can proceed by building the deletable blobs functionality in a following PR.

@giac-mysten giac-mysten merged commit ec29571 into main Dec 12, 2024
21 checks passed
@giac-mysten giac-mysten deleted the gg/site-builder/update-package-and-burn branch December 12, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants