Skip to content

Commit

Permalink
Fix bug where "use --force" prompt is not displayed
Browse files Browse the repository at this point in the history
For some reason our series selector was not returning an
UnsupportedSeries error when it should have been, instead duplicating
the text of the error and returning a generic error

Return an UnsupportedSeries error when the requested series is not
supported by the charm. This can then be detected downstream.

However, if the series is not supported by Juju, then we return a normal
NotSupported error

Remove some duplicated checks. We do not need validate the output of
charmSeries, since the series selector guarentees the returned series is
supported by juju

Drop selecting from charm url as this is a charmstore only feature

We also drop charmValidationError in a few places, because all this did
was reformat the error from charmSeries. Now I have re-written these
error messages, this is not longer required
  • Loading branch information
jack-w-shaw committed Oct 6, 2023
1 parent 3a8220e commit 49694bf
Show file tree
Hide file tree
Showing 7 changed files with 116 additions and 90 deletions.
12 changes: 6 additions & 6 deletions cmd/juju/application/deploy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ func (s *DeploySuite) TestDeployFromPath(c *gc.C) {
}

func (s *DeploySuite) TestDeployFromPathUnsupportedSeriesHaveOverlap(c *gc.C) {
// Donot remove this because we want to test: series supported by the charm and series supported by Juju have overlap.
// Do not remove this because we want to test: series supported by the charm and series supported by Juju have overlap.
s.PatchValue(&deployer.SupportedJujuSeries,
func(time.Time, string, string) (set.Strings, error) {
return set.NewStrings(
Expand All @@ -428,11 +428,11 @@ func (s *DeploySuite) TestDeployFromPathUnsupportedSeriesHaveOverlap(c *gc.C) {

path := testcharms.RepoWithSeries("bionic").ClonedDirPath(c.MkDir(), "multi-series")
err := s.runDeploy(c, path, "--base", "ubuntu@12.10")
c.Assert(err, gc.ErrorMatches, `series "quantal" is not supported, supported series are: focal,jammy`)
c.Assert(err, gc.ErrorMatches, `juju does not support series "quantal"`)
}

func (s *DeploySuite) TestDeployFromPathUnsupportedSeriesHaveNoOverlap(c *gc.C) {
// Donot remove this because we want to test: series supported by the charm and series supported by Juju have NO overlap.
// Do not remove this because we want to test: series supported by the charm and series supported by Juju have NO overlap.
s.PatchValue(&deployer.SupportedJujuSeries,
func(time.Time, string, string) (set.Strings, error) {
return set.NewStrings("kinetic"), nil
Expand All @@ -441,7 +441,7 @@ func (s *DeploySuite) TestDeployFromPathUnsupportedSeriesHaveNoOverlap(c *gc.C)

path := testcharms.RepoWithSeries("bionic").ClonedDirPath(c.MkDir(), "multi-series")
err := s.runDeploy(c, path, "--base", "ubuntu@12.10")
c.Assert(err, gc.ErrorMatches, `multi-series is not available on the following series: quantal`)
c.Assert(err, gc.ErrorMatches, `juju does not support series "quantal"`)
}

func (s *DeploySuite) TestDeployFromPathUnsupportedSeriesForce(c *gc.C) {
Expand Down Expand Up @@ -1448,7 +1448,7 @@ func (s *DeploySuite) TestDeployLocalWithSeriesMismatchReturnsError(c *gc.C) {

_, _, err := s.runDeployWithOutput(c, charmDir.Path, "--base", "ubuntu@12.10")

c.Check(err, gc.ErrorMatches, `terms1 is not available on the following series: quantal not supported`)
c.Check(err, gc.ErrorMatches, `juju does not support series "quantal"`)
}

func (s *DeploySuite) TestDeployLocalWithSeriesAndForce(c *gc.C) {
Expand Down Expand Up @@ -1535,7 +1535,7 @@ func (s *DeploySuite) TestDeployLocalWithSupportedNonESMSeries(c *gc.C) {
func (s *DeploySuite) TestDeployLocalWithNotSupportedNonESMSeries(c *gc.C) {
_, loggingPath := s.setupNonESMBase(c)
err := s.runDeploy(c, loggingPath, "--base", "ubuntu@17.10")
c.Assert(err, gc.ErrorMatches, "logging is not available on the following series: artful not supported")
c.Assert(err, gc.ErrorMatches, `juju does not support series "artful"`)
}

// setupConfigFile creates a configuration file for testing set
Expand Down
4 changes: 1 addition & 3 deletions cmd/juju/application/deployer/bundlehandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,6 @@ func (h *bundleHandler) addCharm(change *bundlechanges.AddCharmChange) error {
}

selector := seriesSelector{
charmURLSeries: url.Series,
seriesFlag: change.Params.Series,
supportedSeries: supportedSeries,
supportedJujuSeries: workloadSeries,
Expand Down Expand Up @@ -1000,15 +999,14 @@ func (h *bundleHandler) selectedSeries(ch charm.CharmMeta, chID application.Char

selector := seriesSelector{
seriesFlag: chSeries,
charmURLSeries: chID.URL.Series,
supportedSeries: supportedSeries,
supportedJujuSeries: workloadSeries,
conf: h.modelConfig,
force: h.force,
fromBundle: true,
}
selectedSeries, err := selector.charmSeries()
return selectedSeries, charmValidationError(curl.Name, errors.Trace(err))
return selectedSeries, errors.Trace(err)
}

// scaleApplication updates the number of units for an application.
Expand Down
18 changes: 5 additions & 13 deletions cmd/juju/application/deployer/charm.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/juju/juju/cmd/juju/application/utils"
"github.com/juju/juju/cmd/juju/common"
corebase "github.com/juju/juju/core/base"
corecharm "github.com/juju/juju/core/charm"
"github.com/juju/juju/core/constraints"
"github.com/juju/juju/core/devices"
"github.com/juju/juju/core/instance"
Expand Down Expand Up @@ -418,7 +419,6 @@ func (c *repositoryCharm) PrepareAndDeploy(ctx *cmd.Context, deployAPI DeployerA
}

selector := seriesSelector{
charmURLSeries: userRequestedURL.Series,
seriesFlag: seriesFlag,
supportedSeries: supportedSeries,
supportedJujuSeries: workloadSeries,
Expand All @@ -431,24 +431,15 @@ func (c *repositoryCharm) PrepareAndDeploy(ctx *cmd.Context, deployAPI DeployerA
series, err := selector.charmSeries()
logger.Tracef("Using series %q from %v to deploy %v", series, supportedSeries, userRequestedURL)

imageStream := modelCfg.ImageStream()
// Avoid deploying charm if it's not valid for the model.
// We check this first before possibly suggesting --force.
if err == nil {
if err2 := c.validateCharmSeriesWithName(series, storeCharmOrBundleURL.Name, imageStream); err2 != nil {
return errors.Trace(err2)
}
}

if charm.IsUnsupportedSeriesError(err) {
if corecharm.IsUnsupportedSeriesError(err) {
msg := fmt.Sprintf("%v. Use --force to deploy the charm anyway.", err)
if usingDefaultSeries {
msg += " Used the default-series."
}
return errors.Errorf(msg)
}
if validationErr := charmValidationError(storeCharmOrBundleURL.Name, errors.Trace(err)); validationErr != nil {
return errors.Trace(validationErr)
if err != nil {
return errors.Trace(err)
}

// Ensure we save the origin.
Expand Down Expand Up @@ -514,6 +505,7 @@ func (c *repositoryCharm) PrepareAndDeploy(ctx *cmd.Context, deployAPI DeployerA
// tell different things when deploying a charm and in sake of understanding
// what we deploy, we should converge the two so that both report identical
// values.
imageStream := modelCfg.ImageStream()
if curl != nil && series == "" {
if err := c.validateCharmSeriesWithName(curl.Series, curl.Name, imageStream); err != nil {
return errors.Trace(err)
Expand Down
25 changes: 25 additions & 0 deletions cmd/juju/application/deployer/charm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ func (s *charmSuite) TestRepositoryCharmDeployDryRunDefaultSeriesForce(c *gc.C)
dCharm.validateCharmSeriesWithName = func(series, name string, imageStream string) error {
return nil
}

repoCharm := &repositoryCharm{
deployCharm: *dCharm,
userRequestedURL: s.url,
Expand All @@ -129,6 +130,30 @@ func (s *charmSuite) TestRepositoryCharmDeployDryRunDefaultSeriesForce(c *gc.C)
c.Check(output.String(), gc.Equals, "\"testme\" from charm \"testme\", revision -1 on ubuntu@22.04 would be deployed\n")
}

func (s *charmSuite) TestRepositoryCharmDeployDryRunUnsupportedSeries(c *gc.C) {
ctrl := s.setupMocks(c)
defer ctrl.Finish()
s.resolver = mocks.NewMockResolver(ctrl)
s.expectResolveChannel()
s.expectDeployerAPIModelGet(c, corebase.Base{})

dCharm := s.newDeployCharm()
dCharm.dryRun = true
dCharm.validateCharmSeriesWithName = func(series, name string, imageStream string) error {
return nil
}
dCharm.baseFlag = corebase.MustParseBaseFromString("ubuntu@22.04")

repoCharm := &repositoryCharm{
deployCharm: *dCharm,
userRequestedURL: s.url,
clock: clock.WallClock,
}

err := repoCharm.PrepareAndDeploy(s.ctx, s.deployerAPI, s.resolver)
c.Assert(err, gc.ErrorMatches, `series "jammy" not supported by charm, .* Use --force to deploy the charm anyway.`)
}

func (s *charmSuite) newDeployCharm() *deployCharm {
return &deployCharm{
configOptions: s.configFlag,
Expand Down
2 changes: 1 addition & 1 deletion cmd/juju/application/deployer/deployer.go
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ func (d *factory) maybeReadLocalCharm(getter ModelConfigGetter) (Deployer, error
}

seriesName, err = seriesSelector.charmSeries()
if err = charmValidationError(ch.Meta().Name, errors.Trace(err)); err != nil {
if err != nil {
return nil, errors.Trace(err)
}
}
Expand Down
44 changes: 28 additions & 16 deletions cmd/juju/application/deployer/series_selector.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,6 @@ type modelConfig interface {
type seriesSelector struct {
// seriesFlag is the series passed to the --series flag on the command line.
seriesFlag string
// charmURLSeries is the series specified as part of the charm URL, i.e.
// ch:jammy/ubuntu.
charmURLSeries string
// conf is the configuration for the model we're deploying to.
conf modelConfig
// supportedSeries is the list of series the charm supports.
Expand All @@ -58,10 +55,13 @@ type seriesSelector struct {
// charmSeries determines what series to use with a charm.
// Order of preference is:
// - user requested with --series or defined by bundle when deploying
// - user requested in charm's url (e.g. juju deploy jammy/ubuntu)
// - model default, if set, acts like --series
// - default from charm metadata supported series / series in url
// - default LTS
//
// If charmSeries returns successfully we guarantee:
// - The returned series is supported by juju
// - Unless force is used, the returned series is supported by the charm
func (s seriesSelector) charmSeries() (selectedSeries string, err error) {
// TODO(sidecar): handle systems

Expand All @@ -70,12 +70,6 @@ func (s seriesSelector) charmSeries() (selectedSeries string, err error) {
return s.userRequested(s.seriesFlag)
}

// User specified a series in the charm URL, e.g.
// juju deploy precise/ubuntu.
if s.charmURLSeries != "" {
return s.userRequested(s.charmURLSeries)
}

// No series explicitly requested by the user.
// Use model default series, if explicitly set and supported by the charm.
if defaultBase, explicit := s.conf.DefaultBase(); explicit {
Expand Down Expand Up @@ -130,21 +124,39 @@ func (s seriesSelector) charmSeries() (selectedSeries string, err error) {

// userRequested checks the series the user has requested, and returns it if it
// is supported, or if they used --force.
//
// There are a number of different cases we model here:
// - If the force flag if provided, simply check that this series is valid
// and return it
// - Without force, run SeriesForCharm to deduce which series to use.
// - If this is successful, validate the series and return it
// - If we error with an UnsupportedSeriesError then:
// - Check if the error occurred because the requested series is invalid
// If so, return early with NotSupported
// - Otherwise, attempt to construct a true list of the supported series,
// the intersection of the charm supported and juju supported series,
// and return a new UnsupportedSeriesError. This is so we don't leak
// series juju does not support to the user in our error
// - edge case: If no series supported by the charm are supported by juju,
// return an error indicating this
func (s seriesSelector) userRequested(requestedSeries string) (string, error) {
// TODO(sidecar): handle computed series
series, err := corecharm.SeriesForCharm(requestedSeries, s.supportedSeries)
if s.force {
series = requestedSeries
} else if err != nil {
if corecharm.IsUnsupportedSeriesError(err) {
// Check if the requested series is valid. If it is invalid,
// we do not wish to return an UnsupportedSeriesError. These
// should be used when a charm does not support a requested series
if validSeriesErr := s.validateSeries(requestedSeries); validSeriesErr != nil {
return "", validSeriesErr
}
supported := s.supportedJujuSeries.Intersection(set.NewStrings(s.supportedSeries...))
if supported.IsEmpty() {
return "", errors.NewNotSupported(nil, fmt.Sprintf("series: %s", requestedSeries))
return "", errors.Errorf("the charm defined series %q not supported", strings.Join(s.supportedSeries, ", "))
}
return "", errors.Errorf(
"series %q is not supported, supported series are: %s",
requestedSeries, strings.Join(supported.SortedValues(), ","),
)
return "", corecharm.NewUnsupportedSeriesError(requestedSeries, supported.SortedValues())
}
return "", err
}
Expand All @@ -171,7 +183,7 @@ func (s seriesSelector) validateSeries(seriesName string) error {
}

if !s.supportedJujuSeries.Contains(seriesName) {
return errors.NotSupportedf("series: %s", seriesName)
return errors.NewNotSupported(nil, fmt.Sprintf("juju does not support series %q", seriesName))
}
return nil
}
Loading

0 comments on commit 49694bf

Please sign in to comment.