kadabra, the app auto-updater #268
Reference in New Issue
Block a user
No description provided.
Delete Branch "moritz/abra:update_daemon"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
coop-cloud/organising#236
Autoupdater
kadabrais ready for some reviews.It should run on the server, check for available minor/patch updates and automatically upgrade the apps.
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
kadabrato the.goreleaser.ymlconfiguration for building binaries too?Thanks for working on this!
@ -1,4 +1,5 @@ABRA := ./cmd/abraKADABRA := ./cmd/kadabraIndentation
@ -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"To remove?
@ -0,0 +42,4 @@majorFlag,},Before: internal.SubCommandBefore,Description: `Check for available upgrades`,If you don't add more info on top of the
Usage, then maybe we can skip it?@ -0,0 +50,4 @@logrus.Fatal(err)}// can't import this lib:// stacks := swarm.GetStacks(cl)To remove? Copy/pasta is part of the Go experience 🙃
@ -0,0 +95,4 @@}// Upgrade all appsvar UpgradeAll = cli.Command{Could we merge this code with the
appupgradecommand, rename toupgradeand 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.@ -0,0 +105,4 @@majorFlag,},Before: internal.SubCommandBefore,Description: `Upgrade all deployed apps`,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?@ -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)If possible it'd be good to make the difference clear in the logging, which case is it exactly?
@ -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 {Same for other functions below.
@ -0,0 +161,4 @@if lableValue != "" {value, err := strconv.ParseBool(lableValue)if err != nil {logrus.Fatal(err)Usually we would
return "", errhere and the caller handles thelogrus.Fatal(err)in the command code? It easier to reason about where theerrgets handled later on when we're maintaining the code. Same for the other functions.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.
@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
%wto 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.Now the code is pushed.
@ -0,0 +195,4 @@return envMap}func getLatestUpgrade(cl *dockerclient.Client, stackName string, recipeName string) string {Missing doc string?
@ -0,0 +327,4 @@app := config.App{Name: stackName,Recipe: recipeName,Server: "localhost",Usually
default, maybe aconstto be re-used elsewhere.WIP: auto updaterto WIP: kadabra, the app auto-updater24e751032ato2076de6bb1Really 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 {Nitpick, you could
if !updateAll { ... return nil }and then avoid theelseindentation on the next block.@ -0,0 +204,4 @@logrus.Debugf("no available upgrades for %s", stackName)return "", nil}// Uncomment to select the next version instead of the last versionTo remove or still useful to keep?
Maybe there should be a way like
--nextto 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, errSo e.g. you can do
return nil, fmt.Errorf("getAvailableUpgrades: %s", err)and then in the Command level, use the%wto unwrap the error message so you get e.g.func1: func2: func3: msgwith 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 deploymentfunc createDeployConfig(recipeName string, stackName string, env config.AppEnv) (*composetypes.Config, stack.Deploy, error) {// Workaround, is there a better way?env["STACK_NAME"] = stackNameIf you have accesss to the
config.Appyou canapp.StackName()but if you only have the app, then this is fine? What might be an issue is thatStackName()does some trimming logic? Maybe a good idea to call it before and pass it in?@ -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"Fine to remove this imho, perhaps keep reference to where it came from but remove the
FIXME, I mean?WIP: kadabra, the app auto-updaterto kadabra, the app auto-updater2da26852c6toa155fe1967a155fe1967to026df90ec1