- Notifications
You must be signed in to change notification settings - Fork72
feat(registry/coder/modules): add archive module#422
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
c9efaa3 to90f7dbdCompare90f7dbd to34eca10CompareThere 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've been testing this out locally.
Nice work! Left some comments down below (none blocking).
LMK when this is ready for a more in-depth review.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| - Depends on: `tar` (and `gzip` or `zstd` if you select those compression modes) | ||
| - Installed scripts: | ||
| - `$CODER_SCRIPT_BIN_DIR/coder-archive-create` | ||
| - `$CODER_SCRIPT_BIN_DIR/coder-archive-extract` | ||
| - Library installed to: | ||
| - `$CODER_SCRIPT_DATA_DIR/archive-lib.sh` | ||
| - On start (always): installer decodes and writes the library, then generates the wrappers in `$CODER_SCRIPT_BIN_DIR` with Terraform-provided defaults embedded. | ||
| - Optional on stop: when enabled, a separate `run_on_stop` script invokes the create command. | ||
| ## Features | ||
| - Create a single `.tar`, `.tar.gz`, or `.tar.zst` containing selected paths. | ||
| - Compression algorithms: `gzip`, `zstd`, `none`. | ||
| - Defaults for directory, archive path, compression, include/exclude lists come from Terraform and can be overridden at runtime with CLI flags. | ||
| - Logs and status messages go to stderr; the create command prints only the final archive path to stdout. | ||
| - Strict bash mode and safe invocation of `tar`. | ||
| - Optional: | ||
| - `run_on_stop` to create an archive automatically when the workspace stops. | ||
| - `extract_on_start` to wait for an archive to appear and extract it on start (with timeout). |
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.
Suggestion: you can probably combine these two
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.
Combine which two? I'm worried about the user experience if we assume that extract on start means you also want to run on stop (if that's what you meant).
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.
Sorry -- I meant simply to combine both sets of bullet points, as there seems to be some slight duplication between them.
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 did a major cleanup of the README in822cf15.
Uh oh!
There was an error while loading.Please reload this page.
| paths = [for v in var.paths : replace(v, "/^~/", "$$HOME")] | ||
| exclude_patterns = [for v in var.exclude_patterns : replace(v, "/^~/", "$$HOME")] | ||
| directory = replace(var.directory, "/^~/", "$$HOME") | ||
| output_dir = replace(var.output_dir, "/^~/", "$$HOME") |
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.
If I'm reading this correctly, it only handles the~ at the start of the string? I can't see a reason to have it anywhere else tbh.
Should we also match a path separator to avoid expanding e.g.~username into/home/usernameusername?
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.
We could, but I'll need to look into if tf replace regex handles replacement like"/^~(\/?)/", "$1". Or we can move this into bash to avoid the repetition. Because we'll need to handle both"~", and"~/..."`.
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.
Fixed inb48daf9
Co-authored-by: Cian Johnston <cian@coder.com>
Co-authored-by: Cian Johnston <cian@coder.com>
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.
Nice work! Just a couple of nits below.
| }; | ||
| const fileExists = async (id: string, path: string) => { | ||
| const res = await sh(id, `test -f ${path} && echo yes || echo no`); |
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.
nit: could this just be simplified to
await sh(id, `test -f ${path}; echo $?`);return res.stdout.trim() === "1";| const extractDir = "/tmp/extract"; | ||
| const extract = await bashRun( | ||
| id, | ||
| `${BIN_DIR}/coder-archive-extract -f ${out} -C ${extractDir}`, |
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.
Should we also be validating that these end up in$PATH?
e.g. without${BIN_DIR}
awit bashRun(id, `coder-archive-extract ...`)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.
We can put it in path manually, e.g. setCODER_SCRIPT_BIN_DIR=/usr/local/bin or something. But to make it a truly real test-case, we'd have to be running our commands inside acoder agent spawned environment.
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.
This change adds a new archive module to the Coder registry. It can be used to archive user-data from pre-defined locations and restore it as well.
Where do these locations live? I see in examples that they are archived in /tmp which is not persistent in our example templates. Should we add examples on how we can archive and upload to an external storage like a s2 bucket and then restore from that?
The 2nd suggestion is to move this tocoder-labs namespace.
That's up to the author currently. They can store the archive in a place that's persisted or upload it. Ideally this module can be kept focused on archiving, and uploading/downloading could be handled by another module or scripted by template author. I can add an example how upload/download could be implemented.
Didn't realize we have this, that works 👍🏻 |
Uh oh!
There was an error while loading.Please reload this page.
This change adds a new
archivemodule to the Coder registry. It can be used to archive user-data from pre-defined locations and restore it as well.Here we also explore: