feat!: update command#131
Conversation
|
Love this, thanks! We've had this on the list for a long time 🙂 Let me take a closer look. I think this might deserve to go into a major release, maybe v1? |
aliev
left a comment
aliev
left a comment
There was a problem hiding this comment.
Architecture is clean, the generate/update split is well-scoped. Four blockers inline plus a few non-blocking notes and nits. Happy to see the non-blockers as follow-ups if you'd rather keep this PR tight.
| /// Information about the template source (local path + hash, or git URL + commit/tag). | ||
| pub template: TemplateSourceInfo, | ||
| /// The answers collected during generation, serialised as a JSON value. | ||
| pub answers: serde_json::Value, |
There was a problem hiding this comment.
type: password answers end up here in plaintext, and .baker-generated.yaml usually gets committed. Probably worth dropping password keys before writing, and re-prompting for them on update.
Keychain-based round-tripping would be nicer but feels like a separate PR.
| let tmp = tempfile::TempDir::new()?; | ||
| let tmp_path = tmp.path().to_path_buf(); | ||
| let loaded = clone_git_into_tmp(url, &tmp_path)?; | ||
| std::mem::forget(tmp); |
There was a problem hiding this comment.
mem::forget(tmp) leaks the cloned repo into /tmp on every git-source update. The forget is there because LoadedTemplate.root points inside tmp, so a normal drop would yank the directory mid-run.
Probably worth returning the TempDir alongside LoadedTemplate and holding it on the stack until run() finishes. RAII handles cleanup on its own, including on unwind.
fn fetch_updated_template(...) -> Result<(LoadedTemplate, Option<TempDir>)> { ... }
// in run():
let (loaded, _tmp_guard) = self.fetch_updated_template(...)?;|
|
||
| let original_dir = std::env::current_dir()?; | ||
| std::fs::create_dir_all(parent)?; | ||
| std::env::set_current_dir(parent)?; |
There was a problem hiding this comment.
Couple of things. If GitLoader::load() panics, the trailing set_current_dir(&original_dir) doesn't run, and the process is left with CWD inside a temp dir that's about to be deleted. The CWD_LOCK in update_integration_tests.rs already hints at this. A small RAII guard that restores CWD on Drop covers both panics and any future early-return paths.
The deeper reason this dance exists is that GitLoader::load() clones into <cwd>/<repo_name> rather than an explicit target dir, so update.rs has to nudge CWD to control where the repo lands. Probably worth filing as tech debt: if GitLoader took a target path, this whole helper goes away and the test lock could too.
|
|
||
| if let Some(ref path) = self.args.answers_file { | ||
| let content = std::fs::read_to_string(path)?; | ||
| if let Ok(serde_json::Value::Object(extra)) = |
There was a problem hiding this comment.
Both if let Ok(serde_json::Value::Object(extra)) = ... arms here drop the parse error and the "valid JSON but not an object" case on the floor. A user passing --answers '{"name":}' or a malformed --answers-file gets no feedback, just a silently ignored override.
runner.rs propagates these parse errors via ?, so this is also a behavioral split between generate and update for the same flags. Probably worth using ? here too and returning a clear error when the parsed value isn't an object.
| return Err(crate::error::Error::GeneratedFileNotFound { path }); | ||
| } | ||
| let content = std::fs::read_to_string(&path)?; | ||
| let data: BakerGenerated = serde_yaml::from_str(&content)?; |
There was a problem hiding this comment.
non-blocking: read() doesn't check the version field. Once v2 metadata exists, an old baker will quietly accept it (dropping unknown fields), and a new baker will accept v0.x the same way. Probably worth a if data.version != "1" { return Err(...) } so the mismatch is loud.
| GeneratedFileNotFound { path: std::path::PathBuf }, | ||
|
|
||
| #[error("Template has not changed since last generation (commit/hash unchanged)")] | ||
| TemplateUnchanged, |
There was a problem hiding this comment.
nit: this variant isn't constructed anywhere. The no-change path in update.rs just does println! + Ok(()). Either drop the variant or use it for the early-exit so tests can match on it.
| return Ok(None); | ||
| } | ||
|
|
||
| let execute = self.confirm_hook_execution( |
There was a problem hiding this comment.
non-blocking: runner.rs::confirm_hook_execution rolls pre and post hooks into a single combined prompt ("these hooks will run, ok?"). Here each hook gets its own confirm_hook_execution call, so a template with both pre and post triggers two prompts on update versus one on generate. Probably worth combining them the same way runner does.
| std::fs::create_dir_all(parent)?; | ||
| std::env::set_current_dir(parent)?; | ||
|
|
||
| let result = GitLoader::new(url.to_string(), true).load(); |
There was a problem hiding this comment.
nit: the true here is skip_overwrite_check, but we always clone into a fresh TempDir, so the prompt could never trigger anyway. The flag is functionally a no-op in this call. Maybe a constant or a short comment to make that obvious.
Co-authored-by: Copilot <copilot@github.com>
2 participants
Added breaking changes. With these changes in order to generate a project a generate command is needed. Also added an update command with pulling updates from local and git based project. If the content is not identical git conflict markers are added