-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(turbo): warn on empty cache #8997
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Approving with some minor comments.
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-cache
to 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]