From 9b7c7ccb1514db93cf09a0647b05da6a63e1051b Mon Sep 17 00:00:00 2001 From: Jordan Barrett <90195985+barrettj12@users.noreply.github.com.> Date: Wed, 4 Sep 2024 16:13:28 -0500 Subject: [PATCH] fix: improve error messages for unsupported features 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 commit attempts to improve the error messaging to make it more straightforward. --- .../application/application_unit_test.go | 4 +- .../facades/client/application/deploy_test.go | 3 +- caas/kubernetes/provider/k8s.go | 8 +- core/assumes/error.go | 9 -- core/assumes/error_test.go | 50 +++++++++++ core/assumes/feature_descriptions.go | 16 ---- core/assumes/features.go | 83 +++++++++++++++++++ core/assumes/sat_checker.go | 21 ++--- core/assumes/sat_checker_test.go | 32 ++----- state/stateenvirons/features.go | 6 +- 10 files changed, 156 insertions(+), 76 deletions(-) create mode 100644 core/assumes/error_test.go delete mode 100644 core/assumes/feature_descriptions.go create mode 100644 core/assumes/features.go diff --git a/apiserver/facades/client/application/application_unit_test.go b/apiserver/facades/client/application/application_unit_test.go index 94a4e8466ed..9b1a86bc8f2 100644 --- a/apiserver/facades/client/application/application_unit_test.go +++ b/apiserver/facades/client/application/application_unit_test.go @@ -792,7 +792,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) { diff --git a/apiserver/facades/client/application/deploy_test.go b/apiserver/facades/client/application/deploy_test.go index 588927d4dad..46a1e20ef8d 100644 --- a/apiserver/facades/client/application/deploy_test.go +++ b/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) { diff --git a/caas/kubernetes/provider/k8s.go b/caas/kubernetes/provider/k8s.go index c9cc22159d8..e08b78a8b0a 100644 --- a/caas/kubernetes/provider/k8s.go +++ b/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 } diff --git a/core/assumes/error.go b/core/assumes/error.go index 6232e3a2354..5fed6182461 100644 --- a/core/assumes/error.go +++ b/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(), } diff --git a/core/assumes/error_test.go b/core/assumes/error_test.go new file mode 100644 index 00000000000..8e7669602ac --- /dev/null +++ b/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) + } +} diff --git a/core/assumes/feature_descriptions.go b/core/assumes/feature_descriptions.go deleted file mode 100644 index 1247ec12f14..00000000000 --- a/core/assumes/feature_descriptions.go +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright 2021 Canonical Ltd. -// Licensed under the AGPLv3, see LICENCE file for details. - -package assumes - -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", - } -) diff --git a/core/assumes/features.go b/core/assumes/features.go new file mode 100644 index 00000000000..3b6121f94e6 --- /dev/null +++ b/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, + } +} diff --git a/core/assumes/sat_checker.go b/core/assumes/sat_checker.go index 9c84ff26c6e..1e0993dd9d9 100644 --- a/core/assumes/sat_checker.go +++ b/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() { diff --git a/core/assumes/sat_checker_test.go b/core/assumes/sat_checker_test.go index c4a26ad2cbc..a413fa820e7 100644 --- a/core/assumes/sat_checker_test.go +++ b/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")) diff --git a/state/stateenvirons/features.go b/state/stateenvirons/features.go index f012593717f..8146d37475d 100644 --- a/state/stateenvirons/features.go +++ b/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.