toolshed/abra
3
3
Fork 21
Code Issues 53 Pull Requests 1 Packages Releases 45 Activity

feat: revert all changes during failed release process #791

Merged
decentral1se merged 1 commits from iexos/abra:push-opmswyqxplqx into main 2026-03-03 00:02:16 +00:00
Conversation 2 Commits 1 Files Changed 2 +53 -52
2 changed files with 53 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

82
cli/recipe/release.go
View File

@ -97,6 +97,16 @@ your private key and enter your passphrase beforehand.
log.Fatal(i18n.G("main app service version for %s is empty?", recipe.Name))
}
repo, err := git.PlainOpen(recipe.Dir)
if err != nil {
log.Fatal(err)
}
preCommitHead, err := repo.Head()
if err != nil {
log.Fatal(err)
}
isClean, err := gitPkg.IsClean(recipe.Dir)
if err != nil {
log.Fatal(err)
@ -201,11 +211,6 @@ likely to change.
}
if tagString == "" {
repo, err := git.PlainOpen(recipe.Dir)
if err != nil {
log.Fatal(err)
}
var lastGitTag tagcmp.Tag
iter, err := repo.Tags()
if err != nil {
@ -282,16 +287,6 @@ likely to change.
log.Fatal(i18n.G("invalid version %s specified", tagString))
}
mainService := "app"
label := i18n.G("coop-cloud.${STACK_NAME}.version=%s", tagString)
if !internal.Dry {
if err := recipe.UpdateLabel("compose.y*ml", mainService, label); err != nil {
log.Fatal(err)
}
} else {
log.Info(i18n.G("dry run: not syncing label %s for recipe %s", tagString, recipe.Name))
}
for _, tag := range tags {
previousTagLeftHand := strings.Split(tag, "+")[0]
newTagStringLeftHand := strings.Split(tagString, "+")[0]
@ -300,24 +295,15 @@ likely to change.
}
}
repo, err := git.PlainOpen(recipe.Dir)
if err != nil {
log.Fatal(err)
}
preCommitHead, err := repo.Head()
if err != nil {
log.Fatal(err)
}
if err := createReleaseFromTag(recipe, tagString, mainAppVersion); err != nil {
if cleanErr := cleanTag(recipe, tagString); cleanErr != nil {
log.Fatal(cleanErr)
log.Fatal(i18n.G("unable to clean up tag after failed release attempt: %s", cleanErr))
}
if cleanErr := cleanCommit(recipe, preCommitHead); cleanErr != nil {
log.Fatal(cleanErr)
if resetErr := resetCommit(recipe, preCommitHead); resetErr != nil {
log.Fatal(i18n.G("unable to reset commit after failed release attempt: %s", resetErr))
iexos marked this conversation as resolved Outdated
decentral1se commented 2026-02-27 15:41:20 +00:00
Outdated
Review
Copy Link

In the case of either cleanTag or resetCommit failing while trying to clean up, we'll see a cryptic error without context. Should we adjust the log.Fatal(cleanErr) / log.Fatal(resetErr) to also include something like unable to clean up after failed release attempt: %s

In the case of either `cleanTag` or `resetCommit` failing while trying to clean up, we'll see a cryptic error without context. Should we adjust the `log.Fatal(cleanErr)` / `log.Fatal(resetErr)` to also include something like `unable to clean up after failed release attempt: %s`
👍 1
}
log.Fatal(err)
log.Error(err)
log.Fatal(i18n.G("release failed. any changes made have been reverted"))
}
},
}
@ -375,23 +361,14 @@ func createReleaseFromTag(recipe recipePkg.Recipe, tagString, mainAppVersion str
return err
}
tag, err := tagcmp.Parse(tagString)
if err != nil {
return err
}
if tag.MissingMinor {
tag.Minor = "0"
tag.MissingMinor = false
}
if tag.MissingPatch {
tag.Patch = "0"
tag.MissingPatch = false
}
if tagString == "" {
tagString = fmt.Sprintf("%s+%s", tag.String(), mainAppVersion)
mainService := "app"
label := fmt.Sprintf("coop-cloud.${STACK_NAME}.version=%s", tagString)
iexos marked this conversation as resolved Outdated
decentral1se commented 2026-02-27 15:42:05 +00:00
Outdated
Review
Copy Link

We can remove the i18n.G(...) wrapper since this string doesn't need to be translated.

That was a previous mistake, I think. You can use a standard fmt.Sprintf instead.

We can remove the `i18n.G(...)` wrapper since this string doesn't need to be translated. That was a previous mistake, I think. You can use a standard `fmt.Sprintf` instead.
👍 1
if !internal.Dry {
if err := recipe.UpdateLabel("compose.y*ml", mainService, label); err != nil {
log.Fatal(err)
}
} else {
log.Info(i18n.G("dry run: not syncing label %s for recipe %s", tagString, recipe.Name))
}
if err := addReleaseNotes(recipe, tagString); err != nil {
@ -587,9 +564,10 @@ func pushRelease(recipe recipePkg.Recipe, tagString string) error {
return nil
}
// cleanCommit soft removes the latest release commit. No change are lost the
// the commit itself is removed. This is the equivalent of `git reset HEAD~1`.
func cleanCommit(recipe recipePkg.Recipe, head *plumbing.Reference) error {
// resetCommit hard resets to the state before release was started.
// This will only remove changes made by the release process due to requiring
// a clean working directory.
func resetCommit(recipe recipePkg.Recipe, head *plumbing.Reference) error {
repo, err := git.PlainOpen(recipe.Dir)
if err != nil {
return errors.New(i18n.G("unable to open repo in %s: %s", recipe.Dir, err))
@ -600,12 +578,12 @@ func cleanCommit(recipe recipePkg.Recipe, head *plumbing.Reference) error {
return errors.New(i18n.G("unable to open work tree in %s: %s", recipe.Dir, err))
}
opts := &git.ResetOptions{Commit: head.Hash(), Mode: git.MixedReset}
opts := &git.ResetOptions{Commit: head.Hash(), Mode: git.HardReset}
decentral1se marked this conversation as resolved Outdated
decentral1se commented 2026-02-27 15:44:07 +00:00
Outdated
Review
Copy Link

Does this still guarantee that we still won't lose "non release related changes"? Can this be covered with a separate integration test as well? I'd rather we are safe than sorry in the case of potential data loss issues. abra has a bit of a bad reptuation in this regard 🙃

Does this still guarantee that we still won't lose "non release related changes"? Can this be covered with a separate integration test as well? I'd rather we are safe than sorry in the case of potential data loss issues. `abra` has a bit of a bad reptuation in this regard 🙃
iexos commented 2026-03-02 09:44:12 +00:00
Outdated
Review
Copy Link

There are already tests covering staged or unstaged changes in which case release will bail out. Is that what you mean or is there something else you want to test?

There are already tests covering staged or unstaged changes in which case `release` will bail out. Is that what you mean or is there something else you want to test?
👍 1
if err := worktree.Reset(opts); err != nil {
return errors.New(i18n.G("unable to soft reset %s: %s", recipe.Dir, err))
return errors.New(i18n.G("unable to hard reset %s: %s", recipe.Dir, err))
}
log.Debug(i18n.G("removed freshly created commit"))
log.Debug(i18n.G("reset commit to pre-release state"))
return nil
}

23
tests/integration/recipe_release.bats
View File

@ -217,3 +217,26 @@ teardown() {
assert_failure
assert_output --partial "automagic insertion not supported yet"
}
@test "push during release fails" {
run $ABRA recipe upgrade "$TEST_RECIPE" --no-input --patch --commit
assert_success
run git -C "$ABRA_DIR/recipes/$TEST_RECIPE" show
assert_success
assert_output --partial 'image: nginx:1.21.6'
wantHash="$(_get_current_hash)"
run git -C "$ABRA_DIR/recipes/$TEST_RECIPE" remote set-url origin-ssh "$ABRA_DIR/does/not/exist"
assert_success
run $ABRA recipe release "$TEST_RECIPE" --no-input --patch
assert_failure
assert_output --partial 'failed to publish new release:'
assert_output --partial 'any changes made have been reverted'
assert_equal "$wantHash" "$(_get_current_hash)"
assert_equal "$(_git_status)" ""
}