feat: add archive_path attribute to coderd_template versions#344
feat: add archive_path attribute to coderd_template versions#344jatcod3r wants to merge 10 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f8930debf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a48a37f432
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
We should add a new subtest to TestAccTemplateResource that tests this feature works with a real coderd, if possible.
| }, | ||
| "archive_path": schema.StringAttribute{ | ||
| Optional: true, | ||
| MarkdownDescription: "A path to a `.tar` or `.zip` archive file to upload as the template version source. Mutually exclusive with `directory`. Changes in the archive contents will trigger the creation of a new template version. The archive must not exceed 100 MiB (the Coder server upload limit).", |
There was a problem hiding this comment.
We should probably validate the 100 MiB limit locally?
| } | ||
|
|
||
| // normalizeZip rewrites a zip archive to ensure that all intermediate directory | ||
| // entries exist. Some tools (e.g. Terraform's archive_file data source) produce |
There was a problem hiding this comment.
Given the edge case archive_File presents us, maybe we need an end-to-end test too that uses that? We have an integration test harness for the provider that should support running this sort of test. If it's a pain to do an acceptance test, like the one I mentioned in my other comment (I don't recall how easy it is), just an integration test would be fine.
| "directory_hash": schema.StringAttribute{ | ||
| Computed: true, | ||
| }, | ||
| "archive_path": schema.StringAttribute{ |
There was a problem hiding this comment.
Neither me nor my agent can figure out why your agent used a new attribute here. Any ideas? Why couldn't we just use directory_hash? You could rename it but then you'd need a state migration, but it's not an attribute users would ever be concerned about so I think keeping it as directory_hash for both is fine?
There was a problem hiding this comment.
Did you mean to highlight archive_hash? Cause I agree, we could re-use directory_hash. Figured that I could detect changes based off the archive separately, but thinking about it now, we're really only worried about file content changes. Making that change now.
If you did mean archive_path, then I'm using that for targeting an individual archive file rather than a directory since that doesn't optionally cover standalone files. Unless u think we could modify it to encapsulate standalone archives too?
bb6aabd to
232f639
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2da00fc7e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Implements coder#340. Adds archive_path as an alternative to directory in the versions block of coderd_template, supporting .tar and .zip files. Changes: - Add archive_path (Optional) and archive_hash (Computed) attributes - Validator ensures exactly one of directory/archive_path is set - Plan modifier computes archive_hash from file contents (SHA-256) - normalizeZip adds missing directory entries for archives produced by Terraform's archive_file data source - uploadArchive sends the archive to Coder with correct content type - Warn when switching between archive_path and directory about hidden file inclusion differences - Document 100 MiB server upload limit in archive_path description Tests: - TestComputeArchiveHash (4 cases) - TestArchiveContentType (5 cases) - TestNormalizeZip (2 cases) - TestReconcileVersionIDs extended with ArchiveHashMatching/Changed
Per Terraform framework guidance, validators should not emit errors for unknown values. When either directory or archive_path is unknown (e.g., referencing a computed value from another resource), the XOR check is skipped. Terraform will re-run validators once values resolve during apply.
- Use stdpath.Dir (stdlib path package) instead of filepath.Dir when traversing ZIP entry names. ZIP entries always use forward slashes per spec, and filepath.Dir is OS-specific (would produce backslashes on Windows). - Enhance the directory-to-archive_path switch warning to also mention that automatic tfvars file discovery is not performed for archive uploads, directing users to use tf_vars explicitly.
- Remove archive_hash attribute entirely - Store archive file hash in directory_hash (same as directory hashing) - Simplify contentHash() to only check directory_hash - Both directory and archive uploads now use directory_hash for change detection, which stores the hash of the actual content (not archive metadata) This unifies hash tracking regardless of source type, making the change detection logic simpler and ensuring the hash represents actual template content for both sources.
Validate that uploaded archives do not exceed the 100 MiB server limit before attempting to upload. This provides faster feedback to users with oversized archives and prevents server-side upload failures.
… entries The Coder server's upload handler (coderd/files.go) converts uploaded ZIPs to tar via archive.CreateTarFromZip(), and the tar extractor (provisionersdk.Untar) calls os.MkdirAll(filepath.Dir(target)) for each file entry, creating parent directories automatically. There is no requirement for explicit directory entries in the uploaded ZIP.
b6caa7f to
d014249
Compare
…when unknown - Removed contentHash() helper; inline DirectoryHash.ValueString() at call sites - Replace empty-string fallback with actual hash computation in Create/Update when DirectoryHash is unknown (source path depended on another resource at plan time) - Remove stale archive_hash comments
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Implements #340. Adds archive_path as an alternative to directory in the versions block of coderd_template, supporting .tar and .zip files.
Changes:
Tests: