kadabra, the app auto-updater #268
No reviewers
Labels
No Label
bug
build
ci/cd
contributing
design
documentation
duplicate
enhancement
help wanted
invalid
meta
question
security
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: coop-cloud/abra#268
Loading…
Reference in New Issue
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
kadabra
is 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
kadabra
to the.goreleaser.yml
configuration for building binaries too?Thanks for working on this!
@ -1,4 +1,5 @@
ABRA := ./cmd/abra
KADABRA := ./cmd/kadabra
Indentation
@ -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 apps
var UpgradeAll = cli.Command{
Could we merge this code with the
appupgrade
command, rename toupgrade
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.@ -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 "", err
here and the caller handles thelogrus.Fatal(err)
in the command code? It easier to reason about where theerr
gets 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
%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.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 aconst
to be re-used elsewhere.WIP: auto updaterto WIP: kadabra, the app auto-updater24e751032a
to2076de6bb1
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 {
Nitpick, you could
if !updateAll { ... return nil }
and then avoid theelse
indentation 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 version
To remove or still useful to keep?
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
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
If you have accesss to the
config.App
you 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-updater2da26852c6
toa155fe1967
a155fe1967
to026df90ec1