Add in Recommended Applications (#10)

The use case for this is say cert-manager, which is useless without some
cluster issuers e.g. let's encrypt.

.github/workflows/pull-request.yaml

@@ -52,7 +52,7 @@ jobs:
     - name: Build Generated Code
       run: make generate
     - name: Generated Code Checked In
-      run: "[[ -z $(git status --porcelain) ]]"
+      run: "STAT=$(git status --porcelain); echo $STAT; [[ -z ${STAT} ]]"
     - name: Unit Test
       run: make test-unit
     - name: Archive code coverage results

.golangci.yaml

@@ -21,6 +21,8 @@ linters:
   - goconst
   - perfsprint
   - mnd
+  - exportloopref
+  - execinquery
 linters-settings:
   gci:
     sections:

charts/application/crds/application.unikorn-cloud.org_applicationsets.yaml

@@ -26,7 +26,12 @@ spec:
     name: v1alpha1
     schema:
       openAPIV3Schema:
-        description: ApplicationSet defines a Helm application.
+        description: |-
+          ApplicationSet defines a set of applications.
+          It works like a normal package manager, installing a package will automatically
+          install any dependencies and recommended packages.  Removeing a package will also
+          remove any dependencies and recoomended packages unless they are kept alive by
+          another package in the set.
         properties:
           apiVersion:
             description: |-
@@ -47,6 +52,44 @@ spec:
             type: object
           spec:
             properties:
+              applications:
+                description: Applications is a list of user requested applications
+                  to install.
+                items:
+                  properties:
+                    application:
+                      description: Application is a reference to the typed application.
+                      properties:
+                        apiGroup:
+                          description: |-
+                            APIGroup is the group for the resource being referenced.
+                            If APIGroup is not specified, the specified Kind must be in the core API group.
+                            For any other third-party types, APIGroup is required.
+                          type: string
+                        kind:
+                          description: Kind is the type of resource being referenced
+                          type: string
+                        name:
+                          description: Name is the name of resource being referenced
+                          type: string
+                        namespace:
+                          description: |-
+                            Namespace is the namespace of resource being referenced
+                            Note that when a namespace is specified, a gateway.networking.k8s.io/ReferenceGrant object is required in the referent namespace to allow that namespace's owner to accept the reference. See the ReferenceGrant documentation for details.
+                            (Alpha) This field requires the CrossNamespaceVolumeDataSource feature gate to be enabled.
+                          type: string
+                      required:
+                      - kind
+                      - name
+                      type: object
+                    version:
+                      description: Version is the version of the application.
+                      pattern: ^v?[0-9]+(\.[0-9]+)?(\.[0-9]+)?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?$
+                      type: string
+                  required:
+                  - application
+                  type: object
+                type: array
               pause:
                 description: Pause, if true, will inhibit reconciliation.
                 type: boolean

pkg/provisioners/managers/application/provisioner.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"slices"
 
 	"github.com/spf13/pflag"
 	sat "github.com/spjmurray/go-sat"
@@ -234,6 +233,7 @@ type solverVisitor struct {
 	model        *sat.Model[AppVersion]
 }
 
+//nolint:cyclop
 func (v *solverVisitor) Visit(id string, enqueue func(string)) error {
 	application, ok := v.applications[id]
 	if !ok {
@@ -253,7 +253,8 @@ func (v *solverVisitor) Visit(id string, enqueue func(string)) error {
 	// dependent applications are also installed, but constrained to the allowed
 	// set for this application. This also has the property that if a version has
 	// no satisfiable deps e.g. then it will add a unary clause that prevents the
-	// version from being used.
+	// version from being used.  If a version is installed, it also implies any
+	// recommended packages should be installed too.
 	for version := range application.Versions() {
 		av := AppVersion{application.Name, version.Version}
 
@@ -265,7 +266,7 @@ func (v *solverVisitor) Visit(id string, enqueue func(string)) error {
 
 			depVersions := make([]AppVersion, 0, len(dependantApplication.Spec.Versions))
 
-			for _, depVersion := range slices.Backward(slices.Collect(dependantApplication.Versions())) {
+			for depVersion := range dependantApplication.Versions() {
 				if dependency.Constraints == nil || dependency.Constraints.Check(&depVersion.Version) {
 					depVersions = append(depVersions, AppVersion{dependency.Name, depVersion.Version})
 				}
@@ -275,6 +276,23 @@ func (v *solverVisitor) Visit(id string, enqueue func(string)) error {
 
 			enqueue(dependency.Name)
 		}
+
+		for _, recommendation := range version.Recommends {
+			recommendedApplication, ok := v.applications[recommendation.Name]
+			if !ok {
+				return fmt.Errorf("%w: requested application %s not in catalog", ErrResourceDependency, recommendation.Name)
+			}
+
+			recVersions := make([]AppVersion, 0, len(recommendedApplication.Spec.Versions))
+
+			for recVersion := range recommendedApplication.Versions() {
+				recVersions = append(recVersions, AppVersion{recommendation.Name, recVersion.Version})
+			}
+
+			v.model.ImpliesAtLeastOneOf(av, recVersions...)
+
+			enqueue(recommendation.Name)
+		}
 	}
 
 	return nil
@@ -322,19 +340,10 @@ func SolveApplicationSet(ctx context.Context, client client.Client, namespace st
 			continue
 		}
 
-		// Otherise we must install at least one version.
-		// NOTE: we cheat a bit here, when making a choice the solver will pick
-		// the first undefined variable and set it to true, so we implicitly
-		// choose the most recent version by adding them in a descending order.
-		versions := slices.Collect(application.Versions())
-		if len(versions) == 0 {
-			return nil, fmt.Errorf("%w: requested application %s has no versions", ErrResourceDependency, application.Name)
-		}
-
-		l := make([]AppVersion, len(versions))
+		l := make([]AppVersion, 0, len(application.Spec.Versions))
 
-		for i, version := range slices.Backward(versions) {
-			l[i] = AppVersion{application.Name, version.Version}
+		for version := range application.Versions() {
+			l = append(l, AppVersion{application.Name, version.Version})
 		}
 
 		model.AtLeastOneOf(l...)

pkg/provisioners/managers/application/provisioner_test.go

@@ -86,15 +86,12 @@ type applicationBuilder struct {
 	versions []*unikornv1core.SemanticVersion
 	// dependencies defines a per-version list of package dependencies.
 	dependencies map[string][]unikornv1core.HelmApplicationDependency
-	// recommendations defines a per-version list of package recommendations.
-	recommendations map[string][]unikornv1core.HelmApplicationRecommendation
 }
 
 // newApplicationBuilder createa a new application builder.
 func newApplicationBuilder() *applicationBuilder {
 	return &applicationBuilder{
-		dependencies:    map[string][]unikornv1core.HelmApplicationDependency{},
-		recommendations: map[string][]unikornv1core.HelmApplicationRecommendation{},
+		dependencies: map[string][]unikornv1core.HelmApplicationDependency{},
 	}
 }
 
@@ -116,17 +113,6 @@ func (b *applicationBuilder) withDependency(id string, constraints *unikornv1cor
 	return b
 }
 
-// withRecommendation adds a reccomendation to the current version.
-//
-//nolint:unused
-func (b *applicationBuilder) withRecommendation(id string) *applicationBuilder {
-	b.recommendations[b.currVersion] = append(b.recommendations[b.currVersion], unikornv1core.HelmApplicationRecommendation{
-		Name: id,
-	})
-
-	return b
-}
-
 // get builds and returns the application resource.
 func (b *applicationBuilder) get() *unikornv1core.HelmApplication {
 	app := &unikornv1core.HelmApplication{
@@ -148,10 +134,6 @@ func (b *applicationBuilder) get() *unikornv1core.HelmApplication {
 			v.Dependencies = t
 		}
 
-		if t, ok := b.recommendations[version.Original()]; ok {
-			v.Recommends = t
-		}
-
 		app.Spec.Versions[i] = v
 	}
 
@@ -311,6 +293,8 @@ func TestProvisionSingleWithConflictingTransitveDependency(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// TestProvisionSingleWithChoice makes sure where multiple choices are available
+// and the desirable outcome has a conflict, we satisfy the problem.
 func TestProvisionSingleWithChoice(t *testing.T) {
 	t.Parallel()
 
@@ -348,11 +332,72 @@ func TestProvisionSingleWithChoice(t *testing.T) {
 	order, err := application.Schedule(context.Background(), client, namespace, solution)
 	require.NoError(t, err)
 
+	// TODO: the order of the middle two doesn't really matter as they can be done in
+	// parallel, but it's non-deterministic, so we need a better way of checking this.
+	require.Len(t, order, 4)
+	require.Equal(t, application.NewAppVersion(dep.Name, getSemver(t, "1.0.0")), order[0])
+	require.Equal(t, application.NewAppVersion(app.Name, getSemver(t, "1.0.0")), order[3])
+}
+
+// TestProvisionSingleWithChoiceAndConditionalDependency checks for the "phanom package"
+// problem, where a dependency occurs on a specific version.  The selection heuristic should
+// not install it if it's not explicitly depended upon.
+func TestProvisionSingleWithChoiceAndConditionalDependency(t *testing.T) {
+	t.Parallel()
+
+	dep := newApplicationBuilder().withVersion(getSemver(t, "1.0.0")).get()
+	app := newApplicationBuilder().
+		withVersion(getSemver(t, "1.0.0")).
+		withDependency(dep.Name, getConstraints(t, "=1.0.0")).
+		withVersion(getSemver(t, "2.0.0")).
+		get()
+
+	applicationset := newApplicationSet().withApplication(app.Name, nil).get()
+
+	client := fake.NewClientBuilder().WithScheme(scheme(t)).WithObjects(app, dep).Build()
+	solution, err := application.SolveApplicationSet(context.Background(), client, namespace, applicationset)
+	require.NoError(t, err)
+
+	order, err := application.Schedule(context.Background(), client, namespace, solution)
+	require.NoError(t, err)
+
+	expected := []application.AppVersion{
+		application.NewAppVersion(app.Name, getSemver(t, "2.0.0")),
+	}
+
+	require.Equal(t, expected, order)
+}
+
+// TestProvisionSingleWithRecommendation tests recommendations are correctly picked up
+// and applied.
+func TestProvisionSingleWithRecommendation(t *testing.T) {
+	t.Parallel()
+
+	app := newApplicationBuilder().withVersion(getSemver(t, "1.0.0")).get()
+
+	rec := newApplicationBuilder().
+		withVersion(getSemver(t, "1.0.0")).
+		withDependency(app.Name, getConstraints(t, "=1.0.0")).
+		get()
+
+	app.Spec.Versions[0].Recommends = []unikornv1core.HelmApplicationRecommendation{
+		{
+			Name: rec.Name,
+		},
+	}
+
+	applicationset := newApplicationSet().withApplication(app.Name, nil).get()
+
+	client := fake.NewClientBuilder().WithScheme(scheme(t)).WithObjects(app, rec).Build()
+	solution, err := application.SolveApplicationSet(context.Background(), client, namespace, applicationset)
+	require.NoError(t, err)
+
+	order, err := application.Schedule(context.Background(), client, namespace, solution)
+	require.NoError(t, err)
+
 	expected := []application.AppVersion{
-		application.NewAppVersion(dep.Name, getSemver(t, "1.0.0")),
-		application.NewAppVersion(idep1.Name, getSemver(t, "1.0.0")),
-		application.NewAppVersion(idep2.Name, getSemver(t, "1.0.0")),
 		application.NewAppVersion(app.Name, getSemver(t, "1.0.0")),
+		application.NewAppVersion(rec.Name, getSemver(t, "1.0.0")),
 	}
 
 	require.Equal(t, expected, order)