fix: keep abra working if recipe catalogue is offline #235

moritz · 2022-12-06T16:59:41Z

moritz commented

2022-12-06 16:59:41 +00:00

In case the recipe catalogue server is not accessible most abra commands will not work anymore.

Proposal for coop-cloud/organising#292 :

Show an error and continue with the local recipe catalogue.

This way abra app deploy -C and abra app upgrade -C should still work when the server is down.

In case the recipe catalogue server is not accessible most abra commands will not work anymore. Proposal for https://git.coopcloud.tech/coop-cloud/organising/issues/292 : Show an error and continue with the local recipe catalogue. This way `abra app deploy -C` and `abra app upgrade -C` should still work when the server is down.

❤️ 3

moritz commented

2022-12-07 14:32:01 +00:00

I'm not sure if supressing this error could lead to any unexpected behaviour.
As I could follow the flow, the following commands will use the ReadRecipeCatalogue funciton:

abra app list --status
abra app rollback
abra app upgrade
abra app catalogue generate
abra app version
abra app backup
abra app errors
abra app restore
abra app new
abra recipe lint
abra recipe versions
abra recipe release
abra recipe sync
abra recipe upgrade

I'm not sure if supressing this error could lead to any unexpected behaviour. As I could follow the flow, the following commands will use the `ReadRecipeCatalogue` funciton: - `abra app list --status` - `abra app rollback` - `abra app upgrade` - `abra app catalogue generate` - `abra app version` - `abra app backup` - `abra app errors` - `abra app restore` - `abra app new` - `abra recipe lint` - `abra recipe versions` - `abra recipe release` - `abra recipe sync` - `abra recipe upgrade`

decentral1se referenced this pull request from coop-cloud/organising

2022-12-12 09:52:16 +00:00

If git.coopcloud.tech is down, try to keep going with local files #292

decentral1se commented

2022-12-12 09:59:48 +00:00

@moritz

This is a nice approach! I had a look at the code again, perhaps we could be more specific on the kind of error return value handling from recipeCatalogueFSIsLatest? E.g. when it does:

	httpClient := web.NewHTTPRetryClient()
	res, err := httpClient.Head(RecipeCatalogueURL)
	if err != nil {
		return false, err
	}

And fails, instead of return false, err we can do return false, CatalogueOfflineError() or something. Then we can do an errors.Is(...) check on the err != nil check and do the logrus.Error("unable to retrieve catalogue from internet, using local copy") change as you suggested here? Because some of the errors that recipeCatalogueFSIsLatest are valid and should result in failing to continue.

One thing to check for is the "first use" case, when there is no local catalogue copy. If we do indeed fail to retrieve it from the internet we should check there is a local copy. If not, there isn't much we can do and we should fail to continue? That shouldn't happen too much ofc.

@moritz This is a nice approach! I had a look at the code again, perhaps we could be more specific on the kind of error return value handling from `recipeCatalogueFSIsLatest`? E.g. when it does: ```go httpClient := web.NewHTTPRetryClient() res, err := httpClient.Head(RecipeCatalogueURL) if err != nil { return false, err } ``` And fails, instead of `return false, err` we can do `return false, CatalogueOfflineError()` or something. Then we can do an `errors.Is(...)` check on the `err != nil` check and do the `logrus.Error("unable to retrieve catalogue from internet, using local copy")` change as you suggested here? Because some of the errors that `recipeCatalogueFSIsLatest` are valid and should result in failing to continue. One thing to check for is the "first use" case, when there is no local catalogue copy. If we do indeed fail to retrieve it from the internet we should check there is a local copy. If not, there isn't much we can do and we should fail to continue? That shouldn't happen too much ofc.

moritz commented

2022-12-12 15:24:39 +00:00

Because some of the errors that recipeCatalogueFSIsLatest are valid and should result in failing to continue.

I changed it,so it catches only CatalogueOfflineErrors.

One thing to check for is the "first use" case, when there is no local catalogue copy. If we do indeed fail to retrieve it from the internet we should check there is a local copy. If not, there isn't much we can do and we should fail to continue? That shouldn't happen too much ofc.

The check if there is a local copy is already present: 4e78b060e0/pkg/recipe/recipe.go (L725)

> Because some of the errors that `recipeCatalogueFSIsLatest` are valid and should result in failing to continue. I changed it,so it catches only `CatalogueOfflineErrors`. > One thing to check for is the "first use" case, when there is no local catalogue copy. If we do indeed fail to retrieve it from the internet we should check there is a local copy. If not, there isn't much we can do and we should fail to continue? That shouldn't happen too much ofc. The check if there is a local copy is already present: https://git.coopcloud.tech/coop-cloud/abra/src/commit/4e78b060e0db2b47b4fe0bd2da9cc03db3278afe/pkg/recipe/recipe.go#L725

decentral1se approved these changes 2022-12-12 15:28:05 +00:00

decentral1se left a comment

LGTM