fix: provide a warning when paths provided to save don't exist #104
Conversation
This change has been made to cater for situations where a package manager installation is skipped for some reason. It also avoids cascading errors when a build is misconfigured, as any number of failures will result in these paths not being created.
zhming0
left a comment
zhming0
left a comment
There was a problem hiding this comment.
I left two comments, they are not blockers but I think there might be a better way to approach this change? Keen to hear what you think, if you keen to proceed, I will give it a tick
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestFilterExistingPaths(t *testing.T) { |
There was a problem hiding this comment.
This is an implementation detail. I wonder if we should start by adding an test case for the Save func instead of this internal function?
| // HasWarnings one or more warnings are present in the result | ||
| func (sr *SaveResult) HasWarnings() bool { | ||
| return len(sr.Warnings) > 0 | ||
| } |
There was a problem hiding this comment.
It's not a blocker but I don't anticipate caller side can do anything beyond logging for these warnings. Do you want to consider allowing zstash to take a slog.Logger object and handle logging internally?
There was a problem hiding this comment.
@zhming0 You make a good point, I was tossing up what to do here.
I am really just putting off solving the logger problem, I need to either pass in the slog logger in https://pkg.go.dev/github.com/buildkite/zstash#NewCache or propegate it via the context.
This change has been made to cater for situations where a package manager installation is skipped for some reason. It also avoids cascading errors when a build is misconfigured, as any number of failures will result in these paths not being created.