Merge pull request juju#18022 from barrettj12/assumes-msg

juju#18022

Error messages for supported features were intended to be user-friendly, but they are still too abstract to easily understand. They refer to the concept of "features", which is an internal Juju construct and as such, should not be mentioned to users.

This PR attempts to improve the error messaging to make it more straightforward.

### Before
```console
$ juju deploy nvidia-gpu-operator --channel 1.29/stable
ERROR Charm feature requirements cannot be met:
 - charm requires feature "k8s-api" but model does not support it

Feature descriptions:
 - "k8s-api": the Kubernetes API lets charms query and manipulate the state of API objects in a Kubernetes cluster

For additional information please see: https://juju.is/docs/olm/supported-features
ERROR failed to deploy charm "nvidia-gpu-operator"
```

### After
```console
$ juju deploy assumes-juju
ERROR Charm cannot be deployed because:
 - charm requires Juju version >= 4.0.0, model has version 3.5.4
```

```console
$ juju deploy nvidia-gpu-operator --channel 1.29/stable
ERROR Charm cannot be deployed because:
 - charm must be deployed on a Kubernetes cloud
```

```console
$ juju deploy assumes-k8s-new
ERROR Charm cannot be deployed because:
 - charm requires Kubernetes version >= 1.35.0, model is running on version 1.30.0
```

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- ~[ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

Create a new charm:
```
mkdir assumes-juju
cd assumes-juju
charmcraft init
```
Remove the `containers` and `resources` sections. At the bottom of `charmcraft.yaml`, add the lines:
```yaml
assumes:
- juju >= 4.0
```
Build the charm:
```
charmcraft pack
```
Bootstrap Juju (any cloud) and try to deploy the charm. Check the error message.

Also test the following scenarios:
- deploy a charm that assumes `k8s-api` on a machine cloud (you can use any k8s charm e.g. `nvidia-gpu-operator` for this, no need to build your own)
- deploy a charm that assumes `k8s-api >= 1.35` on a k8s cloud

## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->

**Launchpad bug:** https://bugs.launchpad.net/juju/+bug/2069986

**Jira card:** [JUJU-6584](https://warthogs.atlassian.net/browse/JUJU-6584)

[JUJU-6584]: https://warthogs.atlassian.net/browse/JUJU-6584?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

apiserver/facades/client/application/application_unit_test.go

@@ -791,7 +791,9 @@ func (s *ApplicationSuite) TestSetCharmAssumesNotSatisfied(c *gc.C) {
 		ConfigSettings:  map[string]string{"stringOption": "value"},
 		CharmOrigin:     createCharmOriginFromURL(curl),
 	})
-	c.Assert(err, gc.ErrorMatches, `(?m).*Charm feature requirements cannot be met.*`)
+	c.Assert(err, gc.ErrorMatches, `(?s)Charm cannot be deployed because:
+  - charm requires feature "popcorn" but model does not support it
+`)
 }
 
 func (s *ApplicationSuite) TestSetCharmAssumesNotSatisfiedWithForce(c *gc.C) {

apiserver/facades/client/application/deploy_test.go

@@ -518,7 +518,8 @@ func (s *DeployLocalSuite) TestDeployWithUnmetCharmRequirements(c *gc.C) {
 			NumUnits:        1,
 			CharmOrigin:     corecharm.Origin{Platform: corecharm.Platform{OS: "ubuntu", Channel: "22.04"}},
 		})
-	c.Assert(err, gc.ErrorMatches, "(?m).*Charm feature requirements cannot be met.*")
+	c.Assert(err, gc.ErrorMatches, `(?s)Charm cannot be deployed because:
+  - charm requires Juju version >= 42.0.0.*`)
 }
 
 func (s *DeployLocalSuite) TestDeployWithUnmetCharmRequirementsAndForce(c *gc.C) {

caas/kubernetes/provider/k8s.go

@@ -2615,13 +2615,7 @@ func (k *kubernetesClient) SupportedFeatures() (assumes.FeatureSet, error) {
 		return fs, errors.Annotatef(err, "querying kubernetes API version")
 	}
 
-	fs.Add(
-		assumes.Feature{
-			Name:        "k8s-api",
-			Description: assumes.UserFriendlyFeatureDescriptions["k8s-api"],
-			Version:     k8sAPIVersion,
-		},
-	)
+	fs.Add(assumes.K8sAPIFeature(*k8sAPIVersion))
 	return fs, nil
 }
 

core/assumes/error.go

@@ -62,15 +62,6 @@ func requirementsNotSatisfied(message string, errList []error) *RequirementsNotS
 		}
 	}
 
-	if len(featNames) != 0 {
-		buf.WriteString("\nFeature descriptions:\n")
-		for _, featName := range featNames.SortedValues() {
-			buf.WriteString(fmt.Sprintf("  - %q: %s\n", featName, notSatFeatureDescrs[featName]))
-		}
-	}
-
-	buf.WriteString("\nFor additional information please see: " + featureDocsURL)
-
 	return &RequirementsNotSatisfiedError{
 		message: buf.String(),
 	}

core/assumes/error_test.go

@@ -0,0 +1,50 @@
+// Copyright 2024 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package assumes
+
+import (
+	"fmt"
+
+	"github.com/juju/version/v2"
+	gc "gopkg.in/check.v1"
+)
+
+type errorSuite struct{}
+
+var _ = gc.Suite(&errorSuite{})
+
+var errorTests = []struct {
+	description string
+	featureSet  FeatureSet
+	assumes     string
+	expectedErr string
+}{{
+	description: "Unsupported Juju version",
+	featureSet: FeatureSet{features: map[string]Feature{
+		"juju": JujuFeature(version.MustParse("2.9.42")),
+	}},
+	assumes:     "assumes:\n- juju >= 3.1",
+	expectedErr: "(?s).*charm requires Juju version >= 3.1.0.*",
+}, {
+	description: "Deploying k8s charm on machine cloud",
+	featureSet:  FeatureSet{},
+	assumes:     "assumes:\n- k8s-api",
+	expectedErr: "(?s).*charm must be deployed on a Kubernetes cloud.*",
+}, {
+	description: "k8s version too low",
+	featureSet: FeatureSet{features: map[string]Feature{
+		"k8s-api": K8sAPIFeature(version.MustParse("1.25.0")),
+	}},
+	assumes:     "assumes:\n- k8s-api >= 1.30",
+	expectedErr: "(?s).*charm requires Kubernetes version >= 1.30.*",
+}}
+
+func (*errorSuite) TestErrorMessages(c *gc.C) {
+	for _, test := range errorTests {
+		fmt.Println(test.description)
+		assumesTree := mustParseAssumesExpr(c, test.assumes)
+		err := test.featureSet.Satisfies(assumesTree)
+		c.Check(err, gc.ErrorMatches, test.expectedErr)
+	}
+}

core/assumes/features.go

@@ -0,0 +1,83 @@
+// Copyright 2024 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package assumes
+
+import (
+	"fmt"
+
+	"github.com/juju/version/v2"
+)
+
+var (
+	// A set of user-friendly descriptions for potentially supported
+	// features that are known to the controller. This allows us to
+	// generate better error messages when an "assumes" expression requests a
+	// feature that is not included in the feature set supported by the
+	// current model.
+	UserFriendlyFeatureDescriptions = map[string]string{
+		"juju":    "the version of Juju used by the model",
+		"k8s-api": "the Kubernetes API lets charms query and manipulate the state of API objects in a Kubernetes cluster",
+	}
+)
+
+// featureMissingErrs is a list of user-friendly error messages to return when
+// a given feature is expected by a charm, but not present in the model.
+var featureMissingErrs = map[string]string{
+	"juju":    "charm requires Juju", // this should never happen
+	"k8s-api": "charm must be deployed on a Kubernetes cloud",
+}
+
+// featureMissingErr returns a user-friendly error message to return when a
+// given feature is expected by a charm, but not present in the model.
+func featureMissingErr(featureName string) string {
+	if errStr, ok := featureMissingErrs[featureName]; ok {
+		return errStr
+	}
+	// We don't have a specific error message defined, so return a default.
+	return fmt.Sprintf("charm requires feature %q but model does not support it", featureName)
+}
+
+// featureVersionMismatchErrs is a list of functions which generate
+// user-friendly error messages when a given feature is present in the model,
+// but the version is lower than required by the charm.
+var featureVersionMismatchErrs = map[string]func(constraint, requiredVersion, actualVersion string) string{
+	"juju": func(c, rv, av string) string {
+		return fmt.Sprintf("charm requires Juju version %s %s, model has version %s", c, rv, av)
+	},
+	"k8s-api": func(c, rv, av string) string {
+		return fmt.Sprintf("charm requires Kubernetes version %s %s, model is running on version %s", c, rv, av)
+	},
+}
+
+// featureVersionMismatchErr returns a user-friendly error message to return when
+// a given feature is present in the model, but the version is lower than
+// required by the charm.
+func featureVersionMismatchErr(featureName, constraint, requiredVersion, actualVersion string) string {
+	if f, ok := featureVersionMismatchErrs[featureName]; ok {
+		return f(constraint, requiredVersion, actualVersion)
+	}
+	// We don't have a specific error message defined, so return a default.
+	return fmt.Sprintf("charm requires feature %q (version %s %s) but model currently supports version %s",
+		featureName, constraint, requiredVersion, actualVersion)
+}
+
+// JujuFeature returns a new Feature representing the Juju API for the given
+// version.
+func JujuFeature(ver version.Number) Feature {
+	return Feature{
+		Name:        "juju",
+		Description: UserFriendlyFeatureDescriptions["juju"],
+		Version:     &ver,
+	}
+}
+
+// K8sAPIFeature returns a new Feature representing the Kubernetes API for the
+// given version.
+func K8sAPIFeature(ver version.Number) Feature {
+	return Feature{
+		Name:        "k8s-api",
+		Description: UserFriendlyFeatureDescriptions["k8s-api"],
+		Version:     &ver,
+	}
+}

core/assumes/sat_checker.go

@@ -8,10 +8,6 @@ import (
 	"github.com/juju/errors"
 )
 
-// A link to a web page with additional information about features,
-// the Juju versions that support them etc.
-const featureDocsURL = "https://juju.is/docs/olm/supported-features"
-
 // satisfyExpr checks whether the feature set contents satisfy the provided
 // "assumes" expression. The function can process either feature or composite
 // expressions.
@@ -37,11 +33,9 @@ func satisfyExpr(fs FeatureSet, expr chassumes.Expression, exprTreeDepth int) er
 func satisfyFeatureExpr(fs FeatureSet, expr chassumes.FeatureExpression) error {
 	supported, defined := fs.Get(expr.Name)
 	if !defined {
+		errStr := featureMissingErr(expr.Name)
 		featDescr := UserFriendlyFeatureDescriptions[expr.Name]
-		return featureError(
-			expr.Name, featDescr,
-			"charm requires feature %q but model does not support it", expr.Name,
-		)
+		return featureError(expr.Name, featDescr, errStr)
 	}
 
 	// If the "assumes" feature expression does not specify a version or the
@@ -64,17 +58,16 @@ func satisfyFeatureExpr(fs FeatureSet, expr chassumes.FeatureExpression) error {
 		return nil
 	}
 
+	// Version mismatch. Get the nice error message.
+	errStr := featureVersionMismatchErr(expr.Name, string(expr.Constraint), expr.Version.String(), supported.Version.String())
+
 	var featDescr = supported.Description
 	if featDescr == "" {
 		// The feature set should always have a feature description.
 		// Try the fallback descriptions if it is missing.
 		featDescr = UserFriendlyFeatureDescriptions[featDescr]
 	}
-	return featureError(
-		expr.Name, featDescr,
-		"charm requires feature %q (version %s %s) but model currently supports version %s",
-		expr.Name, expr.Constraint, expr.Version, supported.Version,
-	)
+	return featureError(expr.Name, featDescr, errStr)
 }
 
 // satisfyCompositeExpr checks whether the feature set contents satisfy the
@@ -108,7 +101,7 @@ func satisfyCompositeExpr(fs FeatureSet, expr chassumes.CompositeExpression, exp
 	// below which introduces yet another indentation level and instead
 	// emit a top-level descriptive message.
 	if exprTreeDepth == 0 {
-		return requirementsNotSatisfied("Charm feature requirements cannot be met:", errList)
+		return requirementsNotSatisfied("Charm cannot be deployed because:", errList)
 	}
 
 	switch expr.Type() {

core/assumes/sat_checker_test.go

@@ -30,13 +30,9 @@ assumes:
 `)
 
 	expErr := `
-Charm feature requirements cannot be met:
+Charm cannot be deployed because:
   - charm requires feature "storage" (version >= 2.19.43) but model currently supports version 2.19.42
-
-Feature descriptions:
-  - "storage": create and manipulate storage primitives
-
-For additional information please see: https://juju.is/docs/olm/supported-features`[1:]
+`[1:]
 
 	err := fs.Satisfies(exprTree)
 	c.Assert(err, jc.Satisfies, IsRequirementsNotSatisfiedError, gc.Commentf("expected to get a RequirementsNotSatisfied error"))
@@ -57,19 +53,14 @@ assumes:
 `)
 
 	expErr := `
-Charm feature requirements cannot be met:
+Charm cannot be deployed because:
   - charm requires at least one of the following:
-    - charm requires feature "k8s-api" (version >= 42.0.0) but model currently supports version 1.18.0
+    - charm requires Kubernetes version >= 42.0.0, model is running on version 1.18.0
     - charm requires feature "random-feature-a" but model does not support it
   - charm requires all of the following:
     - charm requires feature "block-storage" but model does not support it
-    - charm requires feature "juju" (version >= 3.0.0) but model currently supports version 2.19.42
-
-Feature descriptions:
-  - "juju": version of Juju running on the controller
-  - "k8s-api": access to the kubernetes API
-
-For additional information please see: https://juju.is/docs/olm/supported-features`[1:]
+    - charm requires Juju version >= 3.0.0, model has version 2.19.42
+`[1:]
 
 	err := fs.Satisfies(exprTree)
 	c.Assert(err, jc.Satisfies, IsRequirementsNotSatisfiedError, gc.Commentf("expected to get a RequirementsNotSatisfied error"))
@@ -98,22 +89,17 @@ assumes:
 `)
 
 	expErr := `
-Charm feature requirements cannot be met:
+Charm cannot be deployed because:
   - charm requires feature "storage" (version >= 42.0.0) but model currently supports version 2.19.42
   - charm requires all of the following:
-    - charm requires feature "juju" (version >= 3.0.0) but model currently supports version 2.19.42
+    - charm requires Juju version >= 3.0.0, model has version 2.19.42
     - charm requires at least one of the following:
       - charm requires feature "random-feature-b" but model does not support it
       - charm requires feature "random-feature-c" but model does not support it
   - charm requires at least one of the following:
     - charm requires feature "random-feature-d" but model does not support it
     - charm requires feature "random-feature-e" but model does not support it
-
-Feature descriptions:
-  - "juju": version of Juju running on the controller
-  - "storage": create and manipulate storage primitives
-
-For additional information please see: https://juju.is/docs/olm/supported-features`[1:]
+`[1:]
 
 	err := fs.Satisfies(exprTree)
 	c.Assert(err, jc.Satisfies, IsRequirementsNotSatisfiedError, gc.Commentf("expected to get a RequirementsNotSatisfied error"))

state/stateenvirons/features.go

@@ -30,11 +30,7 @@ func SupportedFeatures(model Model, newEnviron environs.NewEnvironFunc) (assumes
 	}
 
 	agentVersion, _ := modelConf.AgentVersion()
-	fs.Add(assumes.Feature{
-		Name:        "juju",
-		Description: assumes.UserFriendlyFeatureDescriptions["juju"],
-		Version:     &agentVersion,
-	})
+	fs.Add(assumes.JujuFeature(agentVersion))
 
 	// Access the environment associated with the model and query any
 	// substrate-specific features that might be available.