Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: improve error messages for unsupported features #18022

Merged
merged 1 commit into from
Sep 24, 2024
Merged

fix: improve error messages for unsupported features #18022

merged 1 commit into from
Sep 24, 2024

Conversation

barrettj12
Copy link
Contributor

@barrettj12 barrettj12 commented Sep 4, 2024 •
edited
Loading

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

$ 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

$ juju deploy assumes-juju
ERROR Charm cannot be deployed because:
  - charm requires Juju version >= 4.0.0, model has version 3.5.4
$ juju deploy nvidia-gpu-operator --channel 1.29/stable
ERROR Charm cannot be deployed because:
  - charm must be deployed on a Kubernetes cloud
$ 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

  • Code style: imports ordered, good names, simple structure, etc
  • Comments saying why design decisions were made
  • Go unit tests, with comments saying what you're testing
  • [ ] Integration tests, with comments saying what you're testing
  • [ ] doc.go 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:

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

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

Jira card: JUJU-6584

@barrettj12 barrettj12 marked this pull request as ready for review September 5, 2024 15:27
hmlanigan
hmlanigan reviewed Sep 10, 2024
View reviewed changes
Copy link
Member

@hmlanigan hmlanigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heads up, you'll need to merge this forward explicitly as Simon has a PR up to move the SupportedFeatures function out of stateenvirons on main.

Is bf0a5d3 part of the commit before it? I'm wondering why they need fixing.

core/assumes/sat_checker.go Outdated Show resolved Hide resolved
core/assumes/sat_checker_test.go Show resolved Hide resolved
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why remove the link to documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error messages should be self-explanatory and not require external resources to understand. This was previously necessary because the error message was rather cryptic, and the user would have to consult the docs to find out what it meant.

core/assumes/error_test.go Outdated Show resolved Hide resolved
core/assumes/error_test.go Show resolved Hide resolved
@hpidcock hpidcock added the 3.4 label Sep 10, 2024
@hpidcock
Copy link
Member

This should be targeting 3.5.

@barrettj12
Copy link
Contributor Author

Is bf0a5d3 part of the commit before it? I'm wondering why they need fixing.

The error message has changed, the tests need to be changed accordingly.

@barrettj12 barrettj12 changed the base branch from 3.4 to 3.5 September 17, 2024 13:42
@hpidcock hpidcock added 3.5 and removed 3.4 labels Sep 18, 2024
hmlanigan
hmlanigan reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@hmlanigan hmlanigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please squash commits before merging.

@barrettj12
fix: improve error messages for unsupported features
9b7c7cc 
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.
hmlanigan
hmlanigan approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@hmlanigan hmlanigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to click approved last time.

@barrettj12
Copy link
Contributor Author

/merge

1 similar comment
@barrettj12
Copy link
Contributor Author

/merge

@jujubot jujubot merged commit 1a0fa33 into juju:3.5 Sep 24, 2024
25 of 27 checks passed
@ycliuhw ycliuhw mentioned this pull request Sep 30, 2024
Merged
jujubot added a commit that referenced this pull request Oct 1, 2024
@jujubot
Merge pull request #18155 from ycliuhw/merge-3.5
2af7bd8 
#18155

Forward merge:

- #18154
- #18022
- #17699
- #18095
- #18052
- #18073
- #18091
- #18083

Conflicts:
worker/uniter/runner/context/context_test.go
version/version.go
scripts/win-installer/setup.iss
snap/snapcraft.yaml

Note: this PR also has a small change for the #18154 to include the checksum for the idempotent secret feature.
@ycliuhw ycliuhw mentioned this pull request Oct 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmlanigan hmlanigan hmlanigan approved these changes

@hpidcock hpidcock Awaiting requested review from hpidcock

Assignees
No one assigned
Labels
3.5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@barrettj12 @hpidcock @hmlanigan @jujubot