From d8dbdb0f1601aa7b9c3b492ac86199c898675912 Mon Sep 17 00:00:00 2001 From: Jack Shaw Date: Fri, 18 Aug 2023 11:13:21 +0100 Subject: [PATCH] Respond to comments --- cmd/juju/application/deployer/charm_test.go | 2 +- cmd/juju/application/deployer/deployer.go | 105 +++++++++----------- 2 files changed, 47 insertions(+), 60 deletions(-) diff --git a/cmd/juju/application/deployer/charm_test.go b/cmd/juju/application/deployer/charm_test.go index 033c76386446..e1a4bc8dc2fb 100644 --- a/cmd/juju/application/deployer/charm_test.go +++ b/cmd/juju/application/deployer/charm_test.go @@ -79,7 +79,7 @@ func (s *charmSuite) TestRepositoryCharmDeployDryRunCompatibility(c *gc.C) { dCharm := s.newDeployCharm() dCharm.dryRun = true - dCharm.validateCharmBaseWithName = func(_ corebase.Base, _ string, _ string) error { + dCharm.validateCharmBaseWithName = func(corebase.Base, string, string) error { return nil } repoCharm := &repositoryCharm{ diff --git a/cmd/juju/application/deployer/deployer.go b/cmd/juju/application/deployer/deployer.go index ae9df5604ca9..20541edcfedf 100644 --- a/cmd/juju/application/deployer/deployer.go +++ b/cmd/juju/application/deployer/deployer.go @@ -226,14 +226,17 @@ func (d *factory) localBundleDeployer() (DeployerKind, error) { } func (d *factory) localCharmDeployer(getter ModelConfigGetter) (DeployerKind, error) { - // Determine series + // Determine base charmOrBundle := d.charmOrBundle if isLocalSchema(charmOrBundle) { charmOrBundle = charmOrBundle[6:] } - base, imageStream, seriesErr := d.determineBaseForLocalCharm(charmOrBundle, getter) - if seriesErr != nil { - return nil, errors.Trace(seriesErr) + base, imageStream, baseErr := d.determineBaseForLocalCharm(charmOrBundle, getter) + if errors.Is(baseErr, os.ErrNotExist) { + logger.Debugf("cannot interpret as local charm: %v", baseErr) + return nil, nil + } else if baseErr != nil { + return nil, errors.Trace(baseErr) } // Charm may have been supplied via a path reference. @@ -241,25 +244,24 @@ func (d *factory) localCharmDeployer(getter ModelConfigGetter) (DeployerKind, er // We check for several types of known error which indicate // that the supplied reference was indeed a path but there was // an issue reading the charm located there. - if corecharm.IsMissingSeriesError(err) { + if corecharm.IsMissingBaseError(err) { return nil, err - } else if corecharm.IsUnsupportedSeriesError(err) { + } else if corecharm.IsUnsupportedBaseError(err) { return nil, errors.Trace(err) } else if errors.Cause(err) == zip.ErrFormat { return nil, errors.Errorf("invalid charm or bundle provided at %q", charmOrBundle) } else if errors.Is(err, errors.NotFound) { return nil, errors.Wrap(err, errors.NotFoundf("charm or bundle at %q", charmOrBundle)) - } else if err != nil && err != os.ErrNotExist { + } else if errors.Is(err, os.ErrNotExist) { + logger.Debugf("cannot interpret as local charm: %v", err) + return nil, nil + } else if err != nil { // If we get a "not exists" error then we attempt to interpret // the supplied charm reference as a URL elsewhere, otherwise // we return the error. return nil, errors.Annotatef(err, "attempting to deploy %q", charmOrBundle) - } else if err != nil { - logger.Debugf("cannot interpret as local charm: %v", err) - return nil, nil - } else { - return &localCharmDeployerKind{base, imageStream, ch, curl}, nil } + return &localCharmDeployerKind{base, imageStream, ch, curl}, nil } func (d *factory) localPreDeployedCharmDeployer() (DeployerKind, error) { @@ -277,61 +279,46 @@ func (d *factory) localPreDeployedCharmDeployer() (DeployerKind, error) { } func (d *factory) determineBaseForLocalCharm(charmOrBundle string, getter ModelConfigGetter) (corebase.Base, string, error) { - // TODO (cderici): check the validity of the comments below - // NOTE: Here we select the series using the algorithm defined by - // `baseSelector.charmBase`. This serves to override the algorithm found - // in `charmrepo.NewCharmAtPath` which is outdated (but must still be - // called since the code is coupled with path interpretation logic which - // cannot easily be factored out). - - // NOTE: Reading the charm here is only meant to aid in inferring the - // correct series, if this fails we fall back to the argument series. If - // reading the charm fails here it will also fail below (the charm is read - // again below) where it is handled properly. This is just an expedient to - // get the correct series. A proper refactoring of the charmrepo package is - // needed for a more elegant fix. var ( imageStream string selectedBase corebase.Base ) ch, err := d.charmReader.ReadCharm(charmOrBundle) - if err == nil { - modelCfg, err := getModelConfig(getter) - if err != nil { - return corebase.Base{}, "", errors.Trace(err) - } + if err != nil { + return corebase.Base{}, "", errors.Trace(err) + } - imageStream = modelCfg.ImageStream() - workloadBases, err := SupportedJujuBases(d.clock.Now(), d.base, imageStream) - if err != nil { - return corebase.Base{}, "", errors.Trace(err) - } + modelCfg, err := getModelConfig(getter) + if err != nil { + return corebase.Base{}, "", errors.Trace(err) + } - supportedBases, err := corecharm.ComputedBases(ch) - if err != nil { - return corebase.Base{}, "", errors.Trace(err) - } - if len(supportedBases) == 0 { - logger.Warningf("%s does not declare supported bases", ch.Meta().Name) - } + imageStream = modelCfg.ImageStream() + workloadBases, err := SupportedJujuBases(d.clock.Now(), d.base, imageStream) + if err != nil { + return corebase.Base{}, "", errors.Trace(err) + } - baseSelector, err := corecharm.ConfigureBaseSelector(corecharm.SelectorConfig{ - Config: modelCfg, - Force: d.force, - Logger: logger, - RequestedBase: d.base, - SupportedCharmBases: supportedBases, - WorkloadBases: workloadBases, - UsingImageID: d.constraints.HasImageID() || d.modelConstraints.HasImageID(), - }) - if err != nil { - return corebase.Base{}, "", errors.Trace(err) - } + supportedBases, err := corecharm.ComputedBases(ch) + if err != nil { + return corebase.Base{}, "", errors.Trace(err) + } + baseSelector, err := corecharm.ConfigureBaseSelector(corecharm.SelectorConfig{ + Config: modelCfg, + Force: d.force, + Logger: logger, + RequestedBase: d.base, + SupportedCharmBases: supportedBases, + WorkloadBases: workloadBases, + UsingImageID: d.constraints.HasImageID() || d.modelConstraints.HasImageID(), + }) + if err != nil { + return corebase.Base{}, "", errors.Trace(err) + } - selectedBase, err = baseSelector.CharmBase() - if err = charmValidationError(ch.Meta().Name, errors.Trace(err)); err != nil { - return corebase.Base{}, "", errors.Trace(err) - } + selectedBase, err = baseSelector.CharmBase() + if err = charmValidationError(ch.Meta().Name, errors.Trace(err)); err != nil { + return corebase.Base{}, "", errors.Trace(err) } return selectedBase, imageStream, nil } @@ -569,7 +556,7 @@ func (dk *localPreDeployerKind) CreateDeployer(d factory) (Deployer, error) { } func (dk *localCharmDeployerKind) CreateDeployer(d factory) (Deployer, error) { - // Avoid deploying charm if the charm series is not correct for the + // Avoid deploying charm if the charm base is not correct for the // available image streams. var err error if err = d.validateCharmBaseWithName(dk.base, dk.curl.Name, dk.imageStream); err != nil {