Add -n and -m to recipes/upgrade #291

Merged
cas merged 5 commits from cr_upgrade_noninnteractive into main 2023-04-27 16:46:01 +00:00

View File

@ -2,6 +2,7 @@ package recipe
import (
"bufio"
"encoding/json"
"fmt"
"os"
"path"
@ -27,6 +28,15 @@ type imgPin struct {
version tagcmp.Tag
}
// anUpgrade represents a single service upgrade (as within a recipe), and the list of tags that it can be upgraded to,
cas marked this conversation as resolved
Review

Missing doc string? E.g.

// anUpgrade represents ...
Missing doc string? E.g. ```go // anUpgrade represents ... ```
// for serialization purposes.
type anUpgrade struct {
Service string `json:"service"`
Image string `json:"image"`
Tag string `json:"tag"`
UpgradeTags []string `json:"upgrades"`
}
var recipeUpgradeCommand = cli.Command{
Name: "upgrade",
Aliases: []string{"u"},
@ -56,6 +66,7 @@ You may invoke this command in "wizard" mode and be prompted for input:
internal.PatchFlag,
internal.MinorFlag,
internal.MajorFlag,
internal.MachineReadableFlag,
internal.AllTagsFlag,
},
Before: internal.SubCommandBefore,
@ -75,6 +86,13 @@ You may invoke this command in "wizard" mode and be prompted for input:
}
}
if internal.MachineReadable {
// -m implies -n in this case
internal.NoInput = true
}
upgradeList := make(map[string]anUpgrade)
// check for versions file and load pinned versions
versionsPresent := false
recipeDir := path.Join(config.RECIPES_DIR, recipe.Name)
@ -241,14 +259,37 @@ You may invoke this command in "wizard" mode and be prompted for input:
}
}
prompt := &survey.Select{
Message: msg,
Help: "enter / return to confirm, choose 'skip' to not upgrade this tag, vim mode is enabled",
VimMode: true,
Options: compatibleStrings,
// there is always at least the item "skip" in compatibleStrings (a list of
// possible upgradable tags) and at least one other tag.
upgradableTags := compatibleStrings[1:]
cas marked this conversation as resolved
Review

Maybe sticking len(compatibleStrings[1:]) in a named variable will be more readable since it is used twice. The 1:... is sure to not stack trace since there is always 1 entry in it? We could comment that out also.

Maybe sticking `len(compatibleStrings[1:])` in a named variable will be more readable since it is used twice. The `1:...` is sure to not stack trace since there is always 1 entry in it? We could comment that out also.
Review

The len() is less useful than the slice, I've made that change.

The `len()` is less useful than the slice, I've made that change.
upgrade := anUpgrade{
Service: service.Name,
Image: image,
Tag: tag.String(),
UpgradeTags: make([]string, len(upgradableTags)),
}
if err := survey.AskOne(prompt, &upgradeTag); err != nil {
logrus.Fatal(err)
for n, s := range upgradableTags {
var sb strings.Builder
if _, err := sb.WriteString(s); err != nil {
}
upgrade.UpgradeTags[n] = sb.String()
}
upgradeList[upgrade.Service] = upgrade
if internal.NoInput {
cas marked this conversation as resolved
Review

Should we make this a const somewhere? Think it's getting re-used in several places now.

Should we make this a `const` somewhere? Think it's getting re-used in several places now.
Review

We should do but I don't want to muddle this in with the change.

We should do but I don't want to muddle this in with the change.
upgradeTag = "skip"
} else {
prompt := &survey.Select{
Message: msg,
Help: "enter / return to confirm, choose 'skip' to not upgrade this tag, vim mode is enabled",
VimMode: true,
Options: compatibleStrings,
}
if err := survey.AskOne(prompt, &upgradeTag); err != nil {
logrus.Fatal(err)
}
}
}
}
@ -261,10 +302,30 @@ You may invoke this command in "wizard" mode and be prompted for input:
logrus.Infof("tag upgraded from %s to %s for %s", tag.String(), upgradeTag, image)
}
} else {
logrus.Warnf("not upgrading %s, skipping as requested", image)
if !internal.NoInput {
logrus.Warnf("not upgrading %s, skipping as requested", image)
}
}
}
if internal.NoInput {
cas marked this conversation as resolved
Review

This is a dangling else, it can be dropped?

This is a dangling else, it can be dropped?
Review

yep! done

yep! done
if internal.MachineReadable {
jsonstring, err := json.Marshal(upgradeList)
if err != nil {
logrus.Fatal(err)
cas marked this conversation as resolved
Review

For readability, this else could be also dropped if you return earlier in the above block. Up to you.

For readability, this `else` could be also dropped if you `return` earlier in the above block. Up to you.
Review

Yep, does looks cleaner. Done.

Yep, does looks cleaner. Done.
}
fmt.Println(string(jsonstring))
return nil
}
for _, upgrade := range upgradeList {
logrus.Infof("can upgrade service: %s, image: %s, tag: %s ::\n", upgrade.Service, upgrade.Image, upgrade.Tag)
for _, utag := range upgrade.UpgradeTags {
logrus.Infof(" %s\n", utag)
}
}
}
return nil
},
}