Skip to content

Commit

Permalink
Allow series and base in bundles if they match
Browse files Browse the repository at this point in the history 
Add a verification step into the client to ensure that series and base
match.

As such, we can treat series and bases as equivalent when they show up
precedence-wise.

Immidately combine into a base in core. As a helpful side effect, this
means we can minimise our dealing with series in this part of the code,
which will make it very easy to remove completely
  • Loading branch information
@jack-w-shaw
jack-w-shaw committed Aug 21, 2023
1 parent d338beb commit fa4a30e
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 18 deletions.
62 changes: 62 additions & 0 deletions cmd/juju/application/bundle/bundle.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package bundle

import (
"fmt"
"reflect"
"sort"
"strings"
Expand Down Expand Up @@ -299,6 +300,15 @@ func verifyBundle(data *charm.BundleData, bundleDir string) error {
_, err := devices.ParseConstraints(s)
return err
}

// This method cannot be included within data.Verify because
// to verify corresponding series and base match we need to be
// able to compare them. The charm package, however, treats bases
// and series generically and is unable to do this.
if err := verifyMixedSeriesBasesMatch(data); err != nil {
return errors.Trace(err)
}

var verifyError error
if bundleDir == "" {
verifyError = data.Verify(verifyConstraints, verifyStorage, verifyDevices)
Expand All @@ -315,3 +325,55 @@ func verifyBundle(data *charm.BundleData, bundleDir string) error {
}
return errors.Trace(verifyError)
}

func verifyMixedSeriesBasesMatch(data *charm.BundleData) error {
if data == nil {
return nil
}
if data.Series != "" && data.DefaultBase != "" {
b, err := corebase.ParseBaseFromString(data.DefaultBase)
if err != nil {
return errors.Trace(err)
}
s, err := corebase.GetSeriesFromBase(b)
if err != nil {
return errors.Trace(err)
}
if s != data.Series {
return errors.NewNotValid(nil, fmt.Sprintf("bundle series %q and base %q must match if supplied", data.Series, data.DefaultBase))
}
}

for name, m := range data.Machines {
if m != nil && m.Series != "" && m.Base != "" {
b, err := corebase.ParseBaseFromString(m.Base)
if err != nil {
return errors.Trace(err)
}
s, err := corebase.GetSeriesFromBase(b)
if err != nil {
return errors.Trace(err)
}
if s != m.Series {
return errors.NewNotValid(nil, fmt.Sprintf("machine %q series %q and base %q must match if supplied", name, m.Series, m.Base))
}
}
}

for name, app := range data.Applications {
if app != nil && app.Series != "" && app.Base != "" {
b, err := corebase.ParseBaseFromString(app.Base)
if err != nil {
return errors.Trace(err)
}
s, err := corebase.GetSeriesFromBase(b)
if err != nil {
return errors.Trace(err)
}
if s != app.Series {
return errors.NewNotValid(nil, fmt.Sprintf("application %q series %q and base %q must match if supplied", name, app.Series, app.Base))
}
}
}
return nil
}
105 changes: 95 additions & 10 deletions cmd/juju/application/bundle/bundle_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,30 @@ func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleOverlayUnmarshallEr
c.Assert(unmarshallErrors[0], gc.Equals, expectedError)
}

func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleMixingBaseAndSeries(c *gc.C) {
defer s.setupMocks(c).Finish()
bundleData, err := charm.ReadBundleData(strings.NewReader(mixedSeriesBaseBundle))
c.Assert(err, jc.ErrorIsNil)
s.expectParts(&charm.BundleDataPart{Data: bundleData})
s.expectBasePath()

obtained, _, err := ComposeAndVerifyBundle(s.bundleDataSource, nil)
c.Assert(err, jc.ErrorIsNil)
c.Assert(obtained, gc.DeepEquals, bundleData)
}

func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleMixingBaseAndSeriesMisMatch(c *gc.C) {
defer s.setupMocks(c).Finish()
bundleData, err := charm.ReadBundleData(strings.NewReader(mixedSeriesBaseBundleMismatch))
c.Assert(err, jc.ErrorIsNil)
s.expectParts(&charm.BundleDataPart{Data: bundleData})
s.expectBasePath()

obtained, _, err := ComposeAndVerifyBundle(s.bundleDataSource, nil)
c.Assert(err, gc.ErrorMatches, `application "wordpress" series "jammy" and base "ubuntu@20.04" must match if supplied`)
c.Assert(obtained, gc.IsNil)
}

func (s *composeAndVerifyRepSuite) setupOverlayFile(c *gc.C) {
s.overlayDir = c.MkDir()
s.overlayFile = filepath.Join(s.overlayDir, "config.yaml")
Expand Down Expand Up @@ -375,59 +399,120 @@ func (m stringSliceMatcher) String() string {
}

const wordpressBundle = `
series: bionic
default-base: ubuntu@22.04
applications:
mysql:
charm: ch:mysql
revision: 42
channel: stable
series: xenial
base: ubuntu@20.04
num_units: 1
to:
- "0"
wordpress:
charm: ch:wordpress
channel: stable
revision: 47
series: xenial
base: ubuntu@20.04
num_units: 1
to:
- "1"
machines:
"0":
series: xenial
base: ubuntu@20.04
"1":
series: xenial
base: ubuntu@20.04
relations:
- - wordpress:db
- mysql:db
`

const typoBundle = `
sries: bionic
sries: jammy
applications:
mysql:
charm: ch:mysql
revision: 42
channel: stable
series: xenial
num_units: 1
to:
- "0"
wordpress:
charm: ch:wordpress
channel: stable
revision: 47
series: xenial
num_units: 1
to:
- "1"
machines:
"0":
series: xenial
constrai: arch=arm64
"1":
series: xenial
relations:
- - wordpress:db
- mysql:db
`

const mixedSeriesBaseBundle = `
series: jammy
default-base: ubuntu@22.04
applications:
mysql:
charm: ch:mysql
revision: 42
channel: stable
series: focal
base: ubuntu@20.04
num_units: 1
to:
- "0"
wordpress:
charm: ch:wordpress
channel: stable
revision: 47
series: jammy
num_units: 1
to:
- "1"
machines:
"0":
series: focal
base: ubuntu@20.04
"1":
series: jammy
relations:
- - wordpress:db
- mysql:db
`

const mixedSeriesBaseBundleMismatch = `
series: jammy
default-base: ubuntu@22.04
applications:
mysql:
charm: ch:mysql
revision: 42
channel: stable
series: focal
base: ubuntu@20.04
num_units: 1
to:
- "0"
wordpress:
charm: ch:wordpress
channel: stable
revision: 47
series: jammy
base: ubuntu@20.04
num_units: 1
to:
- "1"
machines:
"0":
series: focal
base: ubuntu@20.04
"1":
series: jammy
relations:
- - wordpress:db
- mysql:db
Expand Down
11 changes: 11 additions & 0 deletions core/bundle/changes/changes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3756,6 +3756,17 @@ func (s *changesSuite) TestLocalCharmWithExplicitSeries(c *gc.C) {
charm: %s
series: xenial
`, charmDir)
charmMeta := `
name: multi-series
summary: That's a dummy charm with multi-series.
description: A dummy charm.
series:
- jammy
- focal
- bionic
`[1:]
err := os.WriteFile(filepath.Join(charmDir, "metadata.yaml"), []byte(charmMeta), 0644)
c.Assert(err, jc.ErrorIsNil)
s.assertLocalBundleChanges(c, charmDir, bundleContent, "xenial")
s.assertLocalBundleChangesWithDevices(c, charmDir, bundleContent, "xenial")
}
Expand Down
13 changes: 5 additions & 8 deletions core/bundle/changes/handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ type resolver struct {
func (r *resolver) handleApplications() (map[string]string, error) {
add := r.changes.add
applications := r.bundle.Applications
existing := r.model

var defaultSeries string
if r.bundle.Series != "" {
Expand All @@ -43,7 +44,6 @@ func (r *resolver) handleApplications() (map[string]string, error) {
return nil, errors.Trace(err)
}
}
existing := r.model

charms := make(map[string]string, len(applications))
addedApplications := make(map[string]string, len(applications))
Expand Down Expand Up @@ -1286,7 +1286,7 @@ func applicationKey(charm, arch, series, channel string, revision int) string {
}

// getSeries retrieves the series of a application from the ApplicationSpec or from the
// charm path or URL if provided, otherwise falling back on a default series.
// charm path or URL if provided.
//
// DEPRECATED: This should be all about bases.
func getSeries(application *charm.ApplicationSpec, defaultSeries string) (string, error) {
Expand All @@ -1297,26 +1297,23 @@ func getSeries(application *charm.ApplicationSpec, defaultSeries string) (string
return baseToSeries(application.Base)
}

// Handle local charm paths.
if charm.IsValidLocalCharmOrBundlePath(application.Charm) {
_, charmURL, err := corecharm.NewCharmAtPath(application.Charm, defaultSeries)
if corecharm.IsMissingSeriesError(err) {
// local charm path is valid but the charm doesn't declare a default series.
return defaultSeries, nil
return "", nil
} else if corecharm.IsUnsupportedSeriesError(err) {
// The bundle's default series is not supported by the charm, but we'll
// use it anyway. This is no different to the case above where application.Series
// is used without checking for potential charm incompatibility.
return defaultSeries, nil
return "", nil
} else if err != nil {
return "", errors.Trace(err)
}
// Return the default series from the local charm.
return charmURL.Series, nil
}

// The following is safe because the bundle data is assumed to be already
// verified, and therefore this must be a valid charm URL.
charmURL, err := charm.ParseURL(application.Charm)
if err != nil {
return "", errors.Trace(err)
Expand All @@ -1328,7 +1325,7 @@ func getSeries(application *charm.ApplicationSpec, defaultSeries string) (string
}
return charmURL.Series, nil
}
return defaultSeries, nil
return "", nil
}

func baseToSeries(b string) (string, error) {
Expand Down

0 comments on commit fa4a30e

Please sign in to comment.