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

Thank you so much @ammaratef45! I've tried to send some breadcrumbs about what I think might be the issue, similarly to #706. I think we're getting closer to the culprit on this one but not quite there... see what you think about i!

Thank you so much @ammaratef45! I've tried to send some breadcrumbs about what I think might be the issue, similarly to https://git.coopcloud.tech/toolshed/abra/pulls/706. I think we're getting closer to the culprit on this one but not quite there... see what you think about i!

cli/recipe/sync.go Outdated

						
				@ -299,2 +294,4 @@

					)

				}

				func getLatestVersion(recipe recipePkg.Recipe, catl recipePkg.RecipeCatalogue) string {

decentral1se commented

2025-12-19 10:20:58 +00:00

If you log.Fatal(err) in a function, you should (in the world of Golang) be sending back that error. log.Fatal gets hard to track when you call a function in a function in a function etc. So, I would do here (string, error) and instead of log.Fatal(err) I would do return "", err . In some cases, you can even return "", fmt.Errorf(i18n.G("my cool translated explanation of the error with additional context: %s", err)). See other error handling elsewhere for more context. In general, we need to thread all errors back to the CLI main command function and only there will the log.Fatal happen. This is at least, the idea.

If you `log.Fatal(err)` in a function, you should (in the world of Golang) be sending back that error. `log.Fatal` gets hard to track when you call a function in a function in a function etc. So, I would do here `(string, error)` and instead of `log.Fatal(err)` I would do `return "", err` . In some cases, you can even `return "", fmt.Errorf(i18n.G("my cool translated explanation of the error with additional context: %s", err))`. See other error handling elsewhere for more context. In general, we need to thread all errors back to the CLI main command function and only there will the `log.Fatal` happen. This is at least, the idea.

ammaratef45 marked this conversation as resolved

cli/recipe/sync.go Outdated

						
				@ -301,0 +299,4 @@

					if err != nil {

						log.Fatal(err)

					}

					latestRecipeVersion := "0.0.0"

decentral1se commented

2025-12-19 10:18:37 +00:00

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

@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

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

@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

decentral1se marked this conversation as resolved

ammaratef45 added spent time 1 hour

2025-12-19 14:15:41 +00:00

ammaratef45 changed title from ~~avoiding #732 by checking for empty versions list for recipe sync~~ to WIP: avoiding #732 by checking for empty versions list for recipe sync

2025-12-19 14:16:02 +00:00

ammaratef45 started working 2025-12-19 14:29:37 +00:00

ammaratef45 added 1 commit 2025-12-19 14:50:42 +00:00

return error instead of handling it inside getLatestVersion

continuous-integration/drone/push Build is failing

Details

42f9e6d458

ammaratef45 changed title from ~~WIP: avoiding #732 by checking for empty versions list for recipe sync~~ to avoiding #732 by checking for empty versions list for recipe sync

2025-12-19 14:53:15 +00:00

ammaratef45 worked for 23 minutes

2025-12-19 14:53:32 +00:00

decentral1se approved these changes 2025-12-21 15:37:26 +00:00

decentral1se left a comment

LGTM 💟