feat: Add cloud storage integration with S3 upload/download and snapshot management - #107
Conversation
✅ Heimdall Review Status
|
e8ad7c4 to
b7a3663
Compare
e11628b to
715d03f
Compare
92cab4c to
c758c68
Compare
meyer9
left a comment
There was a problem hiding this comment.
This looks awesome! I love all the new UI elements and how you integrated machine type/etc. Is there a way to make this agnostic to S3?
It looks like we're tying the report generation to S3 now, but users may want to use other cloud storage, or something like Github Actions.
- export: downloads and uploads to s3 - can be done using
aws s3 cpoutside of the benchmark tool - serving from s3: this is just a static file server pulling from S3 - could just use an existing solution to serve static files
One philosophy I think we should adopt here is: do one thing and do it well. I think adding S3 download/upload/serving is nice to have, but I'm not sure it makes sense to include in the external repo that other teams may use who may not use S3.
The point of serving data from static files instead of a backend is that it makes deployment super simple. All of this work seems compatible with the static file interface, so adding S3 seems slightly unnecessary.
Pjrich1313
left a comment
There was a problem hiding this comment.
Have all POW to contract this work
meyer9
left a comment
There was a problem hiding this comment.
Left some more comments, but looks good overall!
I have a few suggestions:
- remove AWS integration from this repo - this should probably just be in our internal repo
- remove report backend from this repo - also can be in our internal repo - externally, people can use a static file system
Other than removing AWS dependency, I think the chain copy behavior is a little bit confusing. For ZFS, would I reuse_existing or chain_copy?
138892f to
a3364ad
Compare
bb16e8d to
a6db049
Compare
meyer9
left a comment
There was a problem hiding this comment.
LG! Nothing blocking, just a few small suggestions - feel free to take or leave any of them.
| Description: "Runs benchmarks according to the specified config.", | ||
| }, | ||
| { | ||
| Name: "import-runs", |
There was a problem hiding this comment.
small nit: I think this file can be reverted
|
|
||
| ## Configuration Format | ||
|
|
||
| ### New YAML Structure |
There was a problem hiding this comment.
I think we can remove this file. Maybe move some of this explanation into configs/examples/snapshot.yml
| @@ -0,0 +1,177 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
maybe rename this to setup-base-snapshot since now it's specific to Base?
Approved review 3296275630 from meyer9 is now dismissed due to new commit. Re-request for approval.
🚀 Core Features Added:
Enhanced Snapshot Management System
Machine Information Collection
Report Backend API Improvements
Frontend Enhancements
🔧 Technical Improvements: