Add snapshots feature #365
Conversation
Bl4ckk98
force-pushed
the
feat/snapshots
branch
from
35e61c9 to
7f501dd
Compare
Bl4ckk98
force-pushed
the
feat/snapshots
branch
from
7f501dd to
cc0d2d2
Compare
Bl4ckk98
changed the title
| snapshotCompression: boolean; | ||
| snapshotPath: string; | ||
| snapshotsInProgress: Record<string, { | ||
| name: string; |
There was a problem hiding this comment.
if this is staying in config, please declare the type for the record (name/timestamp/currentsize/id) separately - similar to how RdpArg is - this will grow in future and I don't want it to blow out in line count/complexity too early
src/renderer/lib/config.ts
Outdated
| snapshotMaxCount: number; | ||
| snapshotCompression: boolean; | ||
| snapshotPath: string; | ||
| snapshotsInProgress: Record<string, { |
There was a problem hiding this comment.
what does snapshotsInProgress mean, is this the snapshot we're in the middle of saving? does the fact it's in progress need to be written to disk?
It might be better to have a separate file/instance to store snapshot info in rather than adding it to config, but I'll leave that to Tibix or Levev to decide
src/renderer/lib/snapshot.ts
Outdated
| if (this.#wbConfig.config.snapshotCompression) { | ||
| logger.info(`Creating compressed snapshot at ${outputFile}...`); | ||
| const hasPigz = await this.#hasCmd("pigz"); | ||
| const cmd = hasPigz |
There was a problem hiding this comment.
not really sure what's going on here. are you flushing the disks in Windows before starting snapshot?
are you just tgz-ing the entire volume?
There was a problem hiding this comment.
This file at this size is too unwieldy and it and the functions within violate cyclomatic complexity metrics.
Ideally the functions would be split into smaller chunks that don't each require access to global state (i.e. make them effectively static - pass in args, return values) and then they can grouped in a way that makes sense and then distributed among a bunch of files in a snapshot subdirectory of lib
| // Step 1: empty destination volume | ||
| logger.info(`Step 1: Emptying volume ${volumeName}...`); | ||
| const t0 = Date.now(); | ||
| await execAsync(`docker run --rm -v "${volumeName}":/target alpine sh -c "rm -rf /target/*"`); |
There was a problem hiding this comment.
May I ask for a little more detail? why do you pull in alpine?
Could you have just used docker export -> docker volume rm -> docker import?
|
Functionally, this works fine for me. One thing I would change is that deleting a snapshot switches the delete button and cancel button, so maybe change that. |
Many thanks <3 gonna change it! |
…he snapshots page, prettied version
3 participants
Have you ever wanted to delete System32 inside Winboat? Now you can!
I tried to keep bash at the bare minimum, however some was still needed.
This is now ready to test!
It's a draft, but any review on what is actually done is appreaciated to improve everything.
Actual features:
The SnapshotsManager gets automatically restarted in case of a path change. This is to avoid Winboat restarts.
"snapshotMaxCount": 3,
"snapshotCompression": true,
"rdpArgs": [],
"snapshotPath": "/home/bl4ckk/.winboat/snapshots"
TO IMPLEMENT:
Yes, I tested deleting System32
Resolves #180