kadabra, the app auto-updater #268

Merged
moritz merged 21 commits from moritz/abra:update_daemon into main 2023-02-08 18:54:06 +00:00
Member

coop-cloud/organising#236

Autoupdater kadabra is ready for some reviews.
It should run on the server, check for available minor/patch updates and automatically upgrade the apps.

https://git.coopcloud.tech/coop-cloud/organising/issues/236 Autoupdater `kadabra` is ready for some reviews. It should run on the server, check for available minor/patch updates and automatically upgrade the apps.
decentral1se reviewed 2023-02-01 16:53:27 +00:00
decentral1se left a comment
Owner

Super exciting 👏

So far, I'd say the major issue I have with this is that the error handling does not conform to the usual "pass all errors back up into command handler code as much as possible", do you think that can be re-worked?

I've not wrapped my head around the entire concept of the auto-updater yet but I think I'm getting there. Would you also be able to prepare some documentation over on docs.coopcloud.tech for this too?

You may want to also add kadabra to the .goreleaser.yml configuration for building binaries too?

Thanks for working on this!

Super exciting 👏 So far, I'd say the major issue I have with this is that the error handling does not conform to the usual "pass all errors back up into command handler code as much as possible", do you think that can be re-worked? I've not wrapped my head around the entire concept of the auto-updater yet but I think I'm getting there. Would you also be able to prepare some documentation over on docs.coopcloud.tech for this too? You may want to also add `kadabra` to the `.goreleaser.yml` configuration for building binaries too? Thanks for working on this!
Makefile Outdated
@ -1,4 +1,5 @@
ABRA := ./cmd/abra
KADABRA := ./cmd/kadabra
Owner

Indentation

Indentation
moritz marked this conversation as resolved
@ -0,0 +20,4 @@
"github.com/docker/docker/api/types/filters"
dockerclient "github.com/docker/docker/client"
// "github.com/docker/cli/cli/command/stack/swarm"
Owner

To remove?

To remove?
moritz marked this conversation as resolved
@ -0,0 +42,4 @@
majorFlag,
},
Before: internal.SubCommandBefore,
Description: `Check for available upgrades`,
Owner

If you don't add more info on top of the Usage, then maybe we can skip it?

If you don't add more info on top of the `Usage`, then maybe we can skip it?
moritz marked this conversation as resolved
@ -0,0 +50,4 @@
logrus.Fatal(err)
}
// can't import this lib:
// stacks := swarm.GetStacks(cl)
Owner

To remove? Copy/pasta is part of the Go experience 🙃

To remove? Copy/pasta is part of the Go experience 🙃
moritz marked this conversation as resolved
@ -0,0 +95,4 @@
}
// Upgrade all apps
var UpgradeAll = cli.Command{
Owner

Could we merge this code with the appupgrade command, rename to upgrade and use a -a/--all flag to trigger upgrade for all? Then you can do some bookkeeping logic for the flag / args to make a decision on what to do? Smaller command surface seems good.

Could we merge this code with the `appupgrade` command, rename to `upgrade` and use a -a/--all flag to trigger upgrade for all? Then you can do some bookkeeping logic for the flag / args to make a decision on what to do? Smaller command surface seems good.
moritz marked this conversation as resolved
@ -0,0 +105,4 @@
majorFlag,
},
Before: internal.SubCommandBefore,
Description: `Upgrade all deployed apps`,
Owner

I think it may make sense to document here that we're accepting a <stack_name> and not an <app_name> because we don't have access to the ~/.abra/... files? Further context on when and where this is supposed to be run?

I think it may make sense to document here that we're accepting a `<stack_name>` and not an `<app_name>` because we don't have access to the `~/.abra/...` files? Further context on when and where this is supposed to be run?
moritz marked this conversation as resolved
@ -0,0 +129,4 @@
upgrade(cl, stackName, recipeName, upgradeVersion)
}
} else {
logrus.Debugf("Don't update %s due to missing recipe name, disabled updates or chaos deployment", stackName)
Owner

If possible it'd be good to make the difference clear in the logging, which case is it exactly?

If possible it'd be good to make the difference clear in the logging, which case is it exactly?
moritz marked this conversation as resolved
@ -0,0 +137,4 @@
}
// Read docker label in the format coop-cloud.${STACK_NAME}.${LABEL}
func getLabel(cl *dockerclient.Client, stackName string, label string) string {
Owner
// getLabel ...

Same for other functions below.

``` // getLabel ... ``` Same for other functions below.
moritz marked this conversation as resolved
@ -0,0 +161,4 @@
if lableValue != "" {
value, err := strconv.ParseBool(lableValue)
if err != nil {
logrus.Fatal(err)
Owner

Usually we would return "", err here and the caller handles the logrus.Fatal(err) in the command code? It easier to reason about where the err gets handled later on when we're maintaining the code. Same for the other functions.

Usually we would `return "", err` here and the caller handles the `logrus.Fatal(err)` in the command code? It easier to reason about where the `err` gets handled later on when we're maintaining the code. Same for the other functions.
Author
Member

Ok I applied this, so all errors are passed to the Command function. Is this the way in go? Because it's quite blowing up the code and for debeggung it seems harder for me to trace where the error originates from.

Ok I applied this, so all errors are passed to the Command function. Is this the way in go? Because it's quite blowing up the code and for debeggung it seems harder for me to trace where the error originates from.
Owner

@moritz thanks! Did you push the code? I'm unsure where the changes are.

Yeh it's pretty verbose but that's how we do it so far and Go is kinda verbose as it is... we started without this approach and then it was the reverse, it was hard to understand where the program was exiting with Fatal(...) calls... in the functions or further down? Now everything ends back in the Command code which is easier to manage?

You may want to use %w to unwrap errors? See https://stackoverflow.com/a/61287626 for more details. Then you can add a note to each error string which gets unwrapped further up the stack so that you can see exactly where the error came from. I don't really use this elsewhere in the code as I didn't even know about it at the time but will probably start doing it from now on.

@moritz thanks! Did you push the code? I'm unsure where the changes are. Yeh it's pretty verbose but that's how we do it so far and Go is kinda verbose as it is... we started without this approach and then it was the reverse, it was hard to understand where the program was exiting with `Fatal(...)` calls... in the functions or further down? Now everything ends back in the Command code which is easier to manage? You may want to use `%w` to unwrap errors? See https://stackoverflow.com/a/61287626 for more details. Then you can add a note to each error string which gets unwrapped further up the stack so that you can see exactly where the error came from. I don't really use this elsewhere in the code as I didn't even know about it at the time but will probably start doing it from now on.
Author
Member

Now the code is pushed.

Now the code is pushed.
moritz marked this conversation as resolved
@ -0,0 +195,4 @@
return envMap
}
func getLatestUpgrade(cl *dockerclient.Client, stackName string, recipeName string) string {
Owner

Missing doc string?

Missing doc string?
moritz marked this conversation as resolved
@ -0,0 +327,4 @@
app := config.App{
Name: stackName,
Recipe: recipeName,
Server: "localhost",
Owner

Usually default, maybe a const to be re-used elsewhere.

Usually `default`, maybe a `const` to be re-used elsewhere.
moritz marked this conversation as resolved
decentral1se changed title from WIP: auto updater to WIP: kadabra, the app auto-updater 2023-02-02 08:52:37 +00:00
moritz force-pushed update_daemon from 24e751032a to 2076de6bb1 2023-02-02 13:42:25 +00:00 Compare
decentral1se approved these changes 2023-02-08 10:08:21 +00:00
decentral1se left a comment
Owner

Really coming together! Final comments are Highly Optional. Don't feel like you have to address them. Could merge this as-is now and get testing. Feel free to merge whenever! I'm gonna try fix the build in the meantime.

Really coming together! Final comments are Highly Optional. Don't feel like you have to address them. Could merge this as-is now and get testing. Feel free to merge whenever! I'm gonna try fix the build in the meantime.
@ -0,0 +98,4 @@
logrus.Fatal(err)
}
if !updateAll {
Owner

Nitpick, you could if !updateAll { ... return nil } and then avoid the else indentation on the next block.

Nitpick, you could `if !updateAll { ... return nil }` and then avoid the `else` indentation on the next block.
moritz marked this conversation as resolved
@ -0,0 +204,4 @@
logrus.Debugf("no available upgrades for %s", stackName)
return "", nil
}
// Uncomment to select the next version instead of the last version
Owner

To remove or still useful to keep?

To remove or still useful to keep?
Author
Member

Maybe there should be a way like --next to choose which version to upgrade to?
I am not sure if it's better to always perform upgrades to the next higher version or to choose the latest. Probably for minor/patch updates using the latest version should be fine.

Maybe there should be a way like `--next` to choose which version to upgrade to? I am not sure if it's better to always perform upgrades to the next higher version or to choose the latest. Probably for minor/patch updates using the latest version should be fine.
@ -0,0 +237,4 @@
deployedVersion string) ([]string, error) {
catl, err := recipe.ReadRecipeCatalogue()
if err != nil {
return nil, err
Owner

So e.g. you can do return nil, fmt.Errorf("getAvailableUpgrades: %s", err) and then in the Command level, use the %w to unwrap the error message so you get e.g. func1: func2: func3: msg with something like https://pkg.go.dev/fmt#Errorf passed to logrus? Not sure on the exact details.

So e.g. you can do `return nil, fmt.Errorf("getAvailableUpgrades: %s", err)` and then in the Command level, use the `%w` to unwrap the error message so you get e.g. `func1: func2: func3: msg` with something like https://pkg.go.dev/fmt#Errorf passed to logrus? Not sure on the exact details.
@ -0,0 +310,4 @@
// createDeployConfig merges and enriches the compose config for the deployment
func createDeployConfig(recipeName string, stackName string, env config.AppEnv) (*composetypes.Config, stack.Deploy, error) {
// Workaround, is there a better way?
env["STACK_NAME"] = stackName
Owner

If you have accesss to the config.App you can app.StackName() but if you only have the app, then this is fine? What might be an issue is that StackName()does some trimming logic? Maybe a good idea to call it before and pass it in?

If you have accesss to the `config.App` you can `app.StackName()` but if you only have the app, then this is fine? What might be an issue is that `StackName()`does some trimming logic? Maybe a good idea to call it before and pass it in?
moritz marked this conversation as resolved
@ -485,2 +486,4 @@
}
}
// FIXME: Copypasta from https://github.com/docker/cli/blob/master/cli/command/stack/swarm/list.go because I could't import "github.com/docker/cli/cli/command/stack/swarm"
Owner

Fine to remove this imho, perhaps keep reference to where it came from but remove the FIXME, I mean?

Fine to remove this imho, perhaps keep reference to where it came from but remove the `FIXME`, I mean?
moritz marked this conversation as resolved
moritz changed title from WIP: kadabra, the app auto-updater to kadabra, the app auto-updater 2023-02-08 18:38:04 +00:00
moritz force-pushed update_daemon from 2da26852c6 to a155fe1967 2023-02-08 18:39:34 +00:00 Compare
moritz force-pushed update_daemon from a155fe1967 to 026df90ec1 2023-02-08 18:46:37 +00:00 Compare
moritz merged commit e76ed771df into main 2023-02-08 18:54:02 +00:00
moritz deleted branch update_daemon 2023-02-08 18:54:26 +00:00
Sign in to join this conversation.
No description provided.