Add `defaultIncludePrereleases` #4165

antgamdia · 2022-01-25T17:44:28Z

Description of the change

Having considered #4085 (comment), the purpose of this PR is twofold:

Add a defaultIncludePrereleases param in the plugin-specific config to decide whether or not we should add the VersionSelectionSemverPrereleases{} when creating the installed pkg.
Refactor a little bit the way the plugin config is created (mainly creating a type) and improve the tests.
1. We weren't checking the actual InstalledPackage being created in a test case, so this PR also includes some minor changes in the test case

Benefits

Until we have a way to configure some options from the UI, this PR introduces a way to configure some plugin-specific options, specifically defaultIncludePrereleases for allowing installing pre-releases by kapp-controller.

Possible drawbacks

N/A

Applicable issues

rel Remaining work for running Kubeapps with Carvel #3849

Additional information

N/A

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2022-01-25T17:58:34Z

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go

+				createdPkgInstall, err := s.getPkgInstall(context.Background(), "default", tc.request.TargetContext.Namespace, createInstalledPackageResponse.InstalledPackageRef.Identifier)
+				if err != nil {
+					t.Fatalf("%+v", err)
+				}
+
+				if got, want := createdPkgInstall, tc.expectedPackageInstall; !cmp.Equal(want, got, ignoreUnexported) {
+					t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(want, got, ignoreUnexported))


Missing assert added in this PR

antgamdia · 2022-01-25T17:59:13Z

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go

+					PackageRef: &packagingv1alpha1.PackageRef{
+						RefName: "tetris.foo.example.com",
+						VersionSelection: &vendirversions.VersionSelectionSemver{
+							Constraints: ">=1.0.0 <1.1.0",


Added test cases for the constraints when passing a defaultUpgradePolicy

antgamdia · 2022-01-25T18:00:17Z

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go

@@ -2885,110 +2891,58 @@ func TestCreateInstalledPackage(t *testing.T) {
 					PackageRef: &packagingv1alpha1.PackageRef{
 						RefName: "tetris.foo.example.com",
 						VersionSelection: &vendirversions.VersionSelectionSemver{
-							Constraints: "1.2.3",
+							Constraints: "1.0.0",
+							Prereleases: &vendirversions.VersionSelectionSemverPrereleases{},


Test case enabling the defaultIncludePrereleases

antgamdia · 2022-01-25T18:01:33Z

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go

+							Key:  "values.yaml",
 						},
 					},
 					},
 					Paused:     false,
 					Canceled:   false,
-					SyncPeriod: &metav1.Duration{(time.Second * 30)},
+					SyncPeriod: nil,
 					NoopDelete: false,
 				},
 				Status: packagingv1alpha1.PackageInstallStatus{
 					GenericStatus: kappctrlv1alpha1.GenericStatus{
-						ObservedGeneration: 1,
-						Conditions: []kappctrlv1alpha1.AppCondition{{
-							Type:    kappctrlv1alpha1.ReconcileSucceeded,
-							Status:  k8scorev1.ConditionTrue,
-							Reason:  "baz",
-							Message: "qux",
-						}},
-						FriendlyDescription: "foo",
-						UsefulErrorMessage:  "Deployed",
+						ObservedGeneration:  0,
+						Conditions:          nil,
+						FriendlyDescription: "",
+						UsefulErrorMessage:  "",
 					},
-					Version:              "1.2.3",
-					LastAttemptedVersion: "1.2.3",
+					Version:              "",
+					LastAttemptedVersion: "",


Minor changes in the test cases to match the actual PackageInstall being created.

antgamdia · 2022-01-25T18:02:31Z

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_utils.go

+	kappControllerPluginParsedConfig struct {
+		defaultUpgradePolicy      upgradePolicy
+		defaultIncludePrereleases bool


Struct holding the plugin's config.

castelblanque · 2022-01-26T09:13:07Z

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server.go

+
+	// parse the configuration options
+	defaultUpgradePolicy := upgradePolicyMapping[pluginConfig.KappController.Packages.V1alpha1.DefaultUpgradePolicy]
+	defaultIncludePrereleases, err := strconv.ParseBool(pluginConfig.KappController.Packages.V1alpha1.DefaultUpgradePolicy)


Is pluginConfig.KappController.Packages.V1alpha1.DefaultUpgradePolicy the right param to parse for defaultIncludePrereleases?

Absolutely right! I didn't notice, thanks!

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

absoludity · 2022-01-26T22:55:49Z

chart/kubeapps/values.yaml

@@ -1638,6 +1638,8 @@ kubeappsapis:
          ## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.defaultUpgradePolicy Default upgrade policy generating version constraints
          ## enum: [ "major", "minor", "patch", "none" ]
          defaultUpgradePolicy: none
+          ## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.defaultIncludePrereleases Default policy for allowing pre-releases to be installed
+          defaultIncludePrereleases: false


Sorry, just a thought: I know I'd suggested a boolean when reviewing your other PR - but it might be just as simple to actually have a value here (ie. rename defaultPrereleasesVersionSelection that is empty (nil) by default, but can be set to {} or whatever? This way, we don't have to later change this config option to support actually setting version selections for pre releases? I'm not suggesting we implement support for them yet, it'd still effectively be a boolean option for now, but one that doesn't need to change later.

Fine either way.

No, it's fine. In fact, after re-reading the carvel issues and docs I've started having some doubts/questions, so I asked them at: https://kubernetes.slack.com/archives/CH8KCCKA5/p1643376802571959

In short, prereleases can have three values:

prereleases: {}: allow any prerelease if the version constraint allows it.

prereleases: nil: disallow any prerelease

Exception: if the version constraint is a prerelease itself:
1. Current behavior: error, it won't install a prerelease if prereleases: nil:
2. Future behavior: install it, it won't be required to set prereleases: {} if the version constraint is a prerelease

prereleases: {Identifiers: []string{"rc"}}: allow only prerelease with "rc" as part of the name if the version constraint allows it.

I'm not suggesting we implement support for them yet, it'd still effectively be a boolean option for now, but one that doesn't need to change later.

It is mostly the same effort, I mean, it is just a matter of appending strings or not. I'll update this PR with the full implementation soon (edit: done). Thanks for the suggestion, much better this way!

Also, I'd like to add (in a separate PR) another default config like allowDowngrading or sth, to add the packaging.carvel.dev/downgradable: " annotation to the PackageInstall objs to allow them to be downgraded: https://carvel.dev/kapp-controller/docs/latest/package-consumer-concepts/#downgrading

Nice, yes, that'd be useful (as a kubeapps config default).

Done: #4192 :)

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com> Conflicts: cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_data_adapters.go 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> Conflicts: cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server.go cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go

Add defaultIncludePrereleases. Refactor plugin config. Update tests

14d3669

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia commented Jan 25, 2022

View reviewed changes

This was referenced Jan 25, 2022

Configurable auto-upgrades policy for Carvel pkgs #4085

Merged

Remaining work for running Kubeapps with Carvel #3849

Closed

trigger ci

fa35651

castelblanque reviewed Jan 26, 2022

View reviewed changes

Fix wrong var assignment

6b7ba20

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

absoludity approved these changes Jan 26, 2022

View reviewed changes

antgamdia added 6 commits January 28, 2022 13:33

Merge branch 'main' into addDefaultIncludePrereleases

e3da448

Allow adding prerelease filters

57980b2

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add docs

a6753e7

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Merge branch 'main'

39209a6

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com> Conflicts: cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_data_adapters.go cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go

Minor fixes

af0055d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Merge branch 'main'

0200724

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

antgamdia merged commit d765c48 into vmware-tanzu:main Jan 31, 2022

antgamdia deleted the addDefaultIncludePrereleases branch January 31, 2022 15:42

antgamdia mentioned this pull request Jan 31, 2022

Add default allow downgrades in carvel pkgs #4192

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `defaultIncludePrereleases` #4165

Add `defaultIncludePrereleases` #4165

antgamdia commented Jan 25, 2022

antgamdia Jan 25, 2022

antgamdia Jan 25, 2022

antgamdia Jan 25, 2022

antgamdia Jan 25, 2022

antgamdia Jan 25, 2022

castelblanque Jan 26, 2022

antgamdia Jan 26, 2022

absoludity Jan 26, 2022 •

edited

antgamdia Jan 28, 2022 •

edited

antgamdia Jan 28, 2022

absoludity Jan 31, 2022

antgamdia Jan 31, 2022

Add defaultIncludePrereleases #4165

Add defaultIncludePrereleases #4165

Conversation

antgamdia commented Jan 25, 2022

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity Jan 26, 2022 • edited

Choose a reason for hiding this comment

antgamdia Jan 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `defaultIncludePrereleases` #4165

Add `defaultIncludePrereleases` #4165

absoludity Jan 26, 2022 •

edited

antgamdia Jan 28, 2022 •

edited