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
Fail early when worktree dirty #25
Conversation
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.
Self-review to aid other reviewers.
if (p == crt.Product{}) { | ||
log.Panicf("SourceHash is empty; Product is empty.") | ||
} | ||
log.Panicf("SourceHash is empty; Product is nonempty: % #v", p) |
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.
This assertion required that crt.Product
did not contain a slice (i.e. for it to be comparable). It's not really a necessary check anymore, since issues with the SourceHash would be flagged up by many different tests anyway.
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.
Looks good, just a couple minor comments :)
Co-authored-by: brian shore <bshore@hashicorp.com>
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.
👍
Justification
Currently, when the worktree state changes from clean to dirty between the primary and verification builds, we end up with a failure about a failed upload due to a missing file. This is confusing and unhelpful. See this slack thread (internal only).
Summary
Quality
All changes to behavior should be accompanied by relevant tests which both document and protect it.
This PR includes: