feat(turbo): warn on empty cache #8997
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Skipped Deployments
|
| /// cache for the task. Enabling this can be a useful safety measure to | ||
| /// ensure proper outputs configuration. | ||
| #[clap(long, value_name = "BOOL", action = ArgAction::Set, default_value = "false", default_missing_value = "true", num_args = 0..=1)] | ||
| pub skip_empty_cache: bool, |
There was a problem hiding this comment.
Not sold on adding this as a flag (or this name if we do), but the behavior is nice to have - Enabling this fixes the case when you set outputs incorrectly, and nothing ends up being cached. This can be problematic because if you don't know this happened, and you don't make changes you can get a cache hit that also nothing, making it look like everything worked.
There was a problem hiding this comment.
I'm not super for just adding the flag. This is a setting that is should generally be set at the repo level. e.g. If CI has --skip-empty-cache a user can poison the cache with a bad run if they don't have it set.
| "warning: no files were found that match the configured \ | ||
| outputs - make sure \"outputs\" are correctly defined in \ | ||
| your `turbo.json` for {}", |
There was a problem hiding this comment.
Do we want to write these into the cached task logs so when restoring this hit from cache you'll see this warning?
| return Ok(OutputSaveResult::SkippedEmpty); | ||
| } | ||
| } else { | ||
| debug!("caching log output only for {:?}", self.task_id); |
There was a problem hiding this comment.
Super minor, but I think this debug log is made unnecessary by the debug log earlier.
| @@ -360,6 +364,21 @@ impl TaskCache { | |||
| globwalk::WalkType::All, | |||
| )?; | |||
|
|
|||
| debug!("files to cache: {:?}", files_to_be_cached.len()); | |||
There was a problem hiding this comment.
Maybe add in the task id here
| debug!("files to cache: {:?}", files_to_be_cached.len()); | |
| debug!("{}: files to cache: {:?}", self.task_id, files_to_be_cached.len()); |
| /// cache for the task. Enabling this can be a useful safety measure to | ||
| /// ensure proper outputs configuration. | ||
| #[clap(long, value_name = "BOOL", action = ArgAction::Set, default_value = "false", default_missing_value = "true", num_args = 0..=1)] | ||
| pub skip_empty_cache: bool, |
There was a problem hiding this comment.
I'm not super for just adding the flag. This is a setting that is should generally be set at the repo level. e.g. If CI has --skip-empty-cache a user can poison the cache with a bad run if they don't have it set.
| Ok(status) => { | ||
| match status { | ||
| OutputSaveResult::SavedEmpty => { | ||
| prefixed_ui.warn(format!( |
There was a problem hiding this comment.
I think this might be a little noisy for the new default behavior? I'm fine with shipping as is, but we might need/want a way to silence this warning.
| OutputSaveResult::SkippedEmpty => { | ||
| prefixed_ui.warn("skipped saving cache - no files found"); | ||
| } | ||
| _ => (), |
There was a problem hiding this comment.
Super minor: Explicitly matching against the other variants gets us a compiler check if we end up adding more variants in the future.
| _ => (), | |
| OutputSaveResult::Saved | OutputSaveResult::Disabled => (), |
|
Closing in favor of #9236! |
Description
Warn when you have specified outputs that do not exist.
You can optionally pass
--skip-empty-cacheto skip caching entirely if the specified outputs do not exist. This should probably be the default, but it would be breaking behavior to enable for now.Co-authored-by: Mehul Kar [email protected]