Skip to content
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

Merged
merged 13 commits into from Jan 11, 2023
Merged

Fail early when worktree dirty #25

merged 13 commits into from Jan 11, 2023

Conversation

samsalisbury
Copy link
Contributor

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

  • All builds done by the action itself are now required to be of clean worktrees. (The CLI still allows dirty builds which is useful when working locally.)
  • The error message about dirty worktrees has been enhanced to list the set of dirty paths so that it's easy to diagnose the issue and fix it.

Quality

All changes to behavior should be accompanied by relevant tests which both document and protect it.

This PR includes:

  • New or updated tests which validate the new behavior. (Thank you!)
  • New or updated behavior which has been manually tested. (Please provide a link to relevant logs if this is the case.)
  • New or updated behavior which is not testable in a reasonable time frame. (Please ask for help if this is the case, more things are testable than we sometimes think!)
  • No new or changed behavior. (Just documentation, configuration, or pure refactoring.)

@samsalisbury samsalisbury marked this pull request as ready for review December 7, 2022 14:28
Copy link
Contributor Author

@samsalisbury samsalisbury left a 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.

action.yml Show resolved Hide resolved
action.yml Show resolved Hide resolved
e2e/test.bats Show resolved Hide resolved
if (p == crt.Product{}) {
log.Panicf("SourceHash is empty; Product is empty.")
}
log.Panicf("SourceHash is empty; Product is nonempty: % #v", p)
Copy link
Contributor Author

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.

pkg/build/tempdirs.go Show resolved Hide resolved
pkg/commands/inspect.go Show resolved Hide resolved
pkg/commands/inspect.go Show resolved Hide resolved
pkg/crt/versionfile.go Show resolved Hide resolved
@samsalisbury samsalisbury requested review from a team, sarahethompson and jeanneryan and removed request for a team December 9, 2022 13:18
Copy link
Contributor

@shore shore left a 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 :)

e2e/test.bats Outdated Show resolved Hide resolved
pkg/commands/inspect.go Outdated Show resolved Hide resolved
samsalisbury and others added 2 commits January 11, 2023 12:05
Co-authored-by: brian shore <bshore@hashicorp.com>
Copy link
Contributor

@shore shore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@samsalisbury samsalisbury changed the title Fail when worktree dirty Fail early when worktree dirty Jan 11, 2023
@samsalisbury samsalisbury merged commit 941a719 into main Jan 11, 2023
@samsalisbury samsalisbury deleted the fail-when-worktree-dirty branch January 11, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants