-
Notifications
You must be signed in to change notification settings - Fork 702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configurable auto-upgrades policy for Carvel pkgs #4085
Conversation
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
chart/kubeapps/README.md
Outdated
@@ -491,6 +491,7 @@ Once you have installed Kubeapps follow the [Getting Started Guide](https://gith | |||
| `kubeappsapis.pluginConfig.core.packages.v1alpha1.versionsInSummary.minor` | Number of minor versions to display in the summary | `3` | | |||
| `kubeappsapis.pluginConfig.core.packages.v1alpha1.versionsInSummary.patch` | Number of patch versions to display in the summary | `3` | | |||
| `kubeappsapis.pluginConfig.core.packages.v1alpha1.timeoutSeconds` | Value to wait for Kubernetes commands to complete | `300` | | |||
| `kubeappsapis.pluginConfig.kappController.packages.v1alpha1.upgradePolicy` | Default upgrade policy generating version constraints () | `patch` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we reference this in Carvel documentation created in #4127 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely yes, I think so. However, I was waiting to confirm this is the chosen solution before writing about it, that's why I haven't added it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FMI, why is "patch" the default, not none? It opens the door to automatic (though) minor upgrades, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm... true, perhaps this behavior is too distant from what an average kubeapps user would expect. Defaulting it back to none
. Thanks!
BTW, I've added some docs here now the other PR got merged
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com> Conflicts: cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server.go cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_utils.go
chart/kubeapps/values.yaml
Outdated
v1alpha1: | ||
## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.upgradePolicy Default upgrade policy generating version constraints | ||
## enum: [ "major", "minor", "patch", "none" ] | ||
upgradePolicy: patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we'll (at some point) enable people to set the upgrade policy via the UX, can we name this defaultUpgradePolicy
? (seems to be how it's being used below already anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally right! Naming is often the most difficult part. I've renamed it todefaultUpgradePolicy
.
// https://github.com/vmware-tanzu/carvel-kapp-controller/issues/116 | ||
// This is to allow prereleases to be also installed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain from the comment exactly what the decision is here? The issue has been closed (fixed almost a year ago), but from the description seems to only be relevant if you want to include prereleases in your version selection? Also, see the later carvel-dev/kapp-controller#300 . So wouldn't that mean that we're making that decision here to include pre-releases, as opposed to not doing so by default?
Looks to me like another config option for defaultIncludePrereleases
or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right... I missed this pending task when defining the list of pending actions. I initially add this VersionSelectionSemverPrereleases{}
to explicitly allow prereleases by default, which is the opposite behavior in kapp-ctrl, just to ensure we supported everything (it's easier to notice something is there when it shouldn't than the other way round :P )
Since it's a simple and similar change as the one I did here, let me merge this PR and create a follow-up one straight away.
Thanks for pointing this out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: #4165
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com> Conflicts: cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Description of the change
Following up #4067 (comment), this PR adds a new
upgradePolicy: none|patch|minor|major
field in the plugin config section in the chart to set the default constraint being generated when installing a Carvel package.Benefits
The
LatestMatchingVersion
field will now notify of any update matching this constraint.Possible drawbacks
Kapp will auto-update the package to the latest version satisfying the constraint.
Applicable issues
LatestMatchingVersion
field properly #4067 (comment)Additional information
N/A