toolshed/abra
4
3
Fork 19
Code Issues 39 Pull Requests 4 Packages Releases 42 Activity

avoiding #732 by checking for empty versions list for recipe sync #738

Merged
decentral1se merged 2 commits from 732 into main 2025-12-21 15:37:33 +00:00
Conversation 3 Commits 2 Files Changed 2 +52 -6
2 changed files with 52 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
cli/recipe/sync.go
View File

@ -1,6 +1,7 @@
package recipe
import (
"errors"
"fmt"
"strconv"
"strings"
@ -19,6 +20,9 @@ import (
"github.com/spf13/cobra"
)
// Errors
var emptyVersionsInCatalogue = errors.New("Catalogue versions list is empty (unexpectedly!)")
// translators: `abra recipe reset` aliases. use a comma separated list of
// aliases with no spaces in between
var recipeSyncAliases = i18n.G("s")
@ -121,20 +125,18 @@ likely to change.
log.Fatal(err)
}
versions, err := recipePkg.GetRecipeCatalogueVersions(recipe.Name, catl)
if err != nil {
log.Fatal(err)
}
changesTable, err := formatter.CreateTable()
if err != nil {
log.Fatal(err)
}
latestRelease := tags[len(tags)-1]
latestRecipeVersion, err := getLatestVersion(recipe, catl)
if err != nil && err != emptyVersionsInCatalogue {
log.Fatal(err)
}
changesTable.Headers(i18n.G("SERVICE"), latestRelease, i18n.G("PROPOSED CHANGES"))
latestRecipeVersion := versions[len(versions)-1]
allRecipeVersions := catl[recipe.Name].Versions
for _, recipeVersion := range allRecipeVersions {
if serviceVersions, ok := recipeVersion[latestRecipeVersion]; ok {
@ -298,3 +300,14 @@ func init() {
i18n.G("increase the patch part of the version"),
)
}
decentral1se marked this conversation as resolved Outdated
decentral1se commented 2025-12-19 10:18:37 +00:00
Outdated
Review
Copy Link

Hey, I think #706 (comment) still applies, nowhere else in the history of the recipe config commons have we ever used 0.0.0 (?) and I fear this might break something else.

Isn't it enough to do the len(versions) > 0 { ...} to avoid the index out of range and return an empty list therefore providing there are no recipe versions for this recipe?

I think the problem goes a bit deeper however because the logic block before should catch the lack of recipe versions and prompt the user to choose an initial version? So, something is failing in the if len(tags) == 0 && nextTag == "" { below?

cli/recipe/sync.go Line 80 in 9e7bc31d4d
if len(tags) == 0 && nextTag == "" {

This is a pretty drive-by review but I think the bug is that nextTag gets handled (the maintainer chooses an initial version) but then the code assumes tags (existing recipe versions) has some entries in it? I hope this is making sense....

Hey, I think https://git.coopcloud.tech/toolshed/abra/pulls/706#issuecomment-28193 still applies, nowhere else in the history of the recipe config commons have we ever used `0.0.0` (?) and I fear this might break something else. Isn't it enough to do the `len(versions) > 0 { ...}` to avoid the `index out of range` and return an empty list therefore providing there are no recipe versions for this recipe? I think the problem goes a bit deeper however because the logic block before should catch the lack of recipe versions and prompt the user to choose an initial version? So, something is failing in the `if len(tags) == 0 && nextTag == "" {` below? https://git.coopcloud.tech/toolshed/abra/src/commit/9e7bc31d4d66b5f161122a33bee3ab9e1dce57ea/cli/recipe/sync.go#L80 This is a pretty drive-by review but I think the bug is that `nextTag` gets handled (the maintainer chooses an initial version) but then the code assumes `tags` (existing recipe versions) has some entries in it? I hope this is making sense....
ammaratef45 commented 2025-12-19 14:15:21 +00:00
Outdated
Review
Copy Link

@decentral1se Hmmm, it seems like if len(tags) == 0 && nextTag == "" isn't working because of the presence of local tags that are not in the catalogue (therefore versions is still an empty list), I wonder if we are fixing more than 1 bug here, will reproduce on a new test recipe and see!

P.S. "0.0.0" wasn't a problem because it was never used, it just provided a placeholder value to avoid indexing an empty list, bad readability though, should probably just return an error and handle that error properly from the caller

@decentral1se Hmmm, it seems like `if len(tags) == 0 && nextTag == "" ` isn't working because of the presence of local tags that are not in the catalogue (therefore versions is still an empty list), I wonder if we are fixing more than 1 bug here, will reproduce on a new test recipe and see! P.S. "0.0.0" wasn't a problem because it was never used, it just provided a placeholder value to avoid indexing an empty list, bad readability though, should probably just return an error and handle that error properly from the caller
ammaratef45 commented 2025-12-19 14:36:35 +00:00
Outdated
Review
Copy Link

Nope, I tried on a new recipe and the error didn't happen, unless @jade you can confirm either the state of your local setup when you encountered the error (did you have local tags?) or provide your abra version to match the error line number to make sure the error is the same as the case I found with shlink

The error message on abra version 0.11.0-beta-1c4abcf is identical on my end (copied below) and lines refer to versions not tags having an empty list when it was being accessed

abra recipe sync shlink                                                   ─╯
panic: runtime error: index out of range [-1]

goroutine 1 [running]:
coopcloud.tech/abra/cli/recipe.init.func15(0x14000256200?, {0x140001a5e70, 0x1, 0x1})
	/home/decentral1se/hack/cc/abra/cli/recipe/sync.go:137 +0x1270
github.com/spf13/cobra.(*Command).execute(0x10683eb00, {0x140001a5e50, 0x1, 0x1})
	/home/decentral1se/hack/cc/abra/vendor/github.com/spf13/cobra/command.go:1019 +0x810
github.com/spf13/cobra.(*Command).ExecuteC(0x14000188608)
	/home/decentral1se/hack/cc/abra/vendor/github.com/spf13/cobra/command.go:1148 +0x350
github.com/spf13/cobra.(*Command).Execute(...)
	/home/decentral1se/hack/cc/abra/vendor/github.com/spf13/cobra/command.go:1071
coopcloud.tech/abra/cli.Run({0x105c3c750, 0xb}, {0x105c3ca40, 0x28})
	/home/decentral1se/hack/cc/abra/cli/run.go:312 +0xeac
main.main()
	/home/decentral1se/hack/cc/abra/cmd/abra/main.go:22 +0xc4

side note, how come the error messages include paths from your machine/user @decentral1se? is it because you build the binaries? 😆

Nope, I tried on a new recipe and the error didn't happen, unless @jade you can confirm either the state of your local setup when you encountered the error (did you have local tags?) or provide your abra version to match the error line number to make sure the error is the same as the case I found with shlink The error message on `abra version 0.11.0-beta-1c4abcf` is identical on my end (copied below) and lines refer to versions not tags having an empty list when it was being accessed ``` abra recipe sync shlink ─╯ panic: runtime error: index out of range [-1] goroutine 1 [running]: coopcloud.tech/abra/cli/recipe.init.func15(0x14000256200?, {0x140001a5e70, 0x1, 0x1}) /home/decentral1se/hack/cc/abra/cli/recipe/sync.go:137 +0x1270 github.com/spf13/cobra.(*Command).execute(0x10683eb00, {0x140001a5e50, 0x1, 0x1}) /home/decentral1se/hack/cc/abra/vendor/github.com/spf13/cobra/command.go:1019 +0x810 github.com/spf13/cobra.(*Command).ExecuteC(0x14000188608) /home/decentral1se/hack/cc/abra/vendor/github.com/spf13/cobra/command.go:1148 +0x350 github.com/spf13/cobra.(*Command).Execute(...) /home/decentral1se/hack/cc/abra/vendor/github.com/spf13/cobra/command.go:1071 coopcloud.tech/abra/cli.Run({0x105c3c750, 0xb}, {0x105c3ca40, 0x28}) /home/decentral1se/hack/cc/abra/cli/run.go:312 +0xeac main.main() /home/decentral1se/hack/cc/abra/cmd/abra/main.go:22 +0xc4 ``` side note, how come the error messages include paths from your machine/user @decentral1se? is it because you build the binaries? 😆
ammaratef45 commented 2025-12-19 14:53:11 +00:00
Outdated
Review
Copy Link

@decentral1se Okay I'm now fairly confident we have the right issue at hand (pending confirmation from @jade on either points asked above), I've addressed your other questions regarding the errors and removed the "0.0.0" part (replaced with an empty string and an error that the caller ignores/tolerates) to avoid confusion.

@decentral1se Okay I'm now fairly confident we have the right issue at hand (pending confirmation from @jade on either points asked above), I've addressed your other questions regarding the errors and removed the "0.0.0" part (replaced with an empty string and an error that the caller ignores/tolerates) to avoid confusion.
🚀 1
func getLatestVersion(recipe recipePkg.Recipe, catl recipePkg.RecipeCatalogue) (string, error) {
versions, err := recipePkg.GetRecipeCatalogueVersions(recipe.Name, catl)
if err != nil {
return "", err
}
if len(versions) > 0 {
return versions[len(versions)-1], nil
}
return "", emptyVersionsInCatalogue
}

33
cli/recipe/sync_test.go Normal file
View File

@ -0,0 +1,33 @@
package recipe
import (
"testing"
recipePkg "coopcloud.tech/abra/pkg/recipe"
"github.com/stretchr/testify/assert"
)
func TestGetLatestVersionReturnsErrorWhenVersionsIsEmpty(t *testing.T) {
recipe := recipePkg.Recipe{}
catalogue := recipePkg.RecipeCatalogue{}
_, err := getLatestVersion(recipe, catalogue)
assert.Equal(t, err, emptyVersionsInCatalogue)
}
func TestGetLatestVersionReturnsLastVersion(t *testing.T) {
recipe := recipePkg.Recipe{
Name: "test",
}
versions := []map[string]map[string]recipePkg.ServiceMeta{
make(map[string]map[string]recipePkg.ServiceMeta),
make(map[string]map[string]recipePkg.ServiceMeta),
}
versions[0]["0.0.3"] = make(map[string]recipePkg.ServiceMeta)
versions[1]["0.0.2"] = make(map[string]recipePkg.ServiceMeta)
catalogue := make(recipePkg.RecipeCatalogue)
catalogue["test"] = recipePkg.RecipeMeta{
Versions: versions,
}
version, _ := getLatestVersion(recipe, catalogue)
assert.Equal(t, version, "0.0.3")
}