Merge pull request #14474 from hmlanigan/validate-bundle-dry-run

#14474

Display bundle unmarshall errors when deploying with --dry-run. A compromise for LP1983386 and LP1984133, a rework of #14427 

## QA steps

Deploy local charms, repository charms and bundles, no changes to be found.

```sh
# Deploy the following bundle without force
name: slurm
description: A suite of codified operations to asssemble, install, deploy, and operate Slurm.
website: https://omnivector-solutions.github.io/osd-documentation/master/
docs: https://omnivector-solutions.github.io/osd-documentation/master/
source: https://github.com/omnivector-solutions/slurm-bundles/
issues: https://github.com/omnivector-solutions/slurm-charms/issues

series: focal

applications:
 slurmctld:
 charm: slurmctld
 num_units: 1
 contstraints: instance-type=c5a.large root-disk=20G

$ juju deploy ./bundle-min.yaml --dry-run
WARNING These fields
 document 0:
 line 1: unrecognized field "name"
 line 3: unrecognized field "website"
 line 4: unrecognized field "docs"
 line 5: unrecognized field "source"
 line 6: unrecognized field "issues"
 line 14: unrecognized field "contstraints"
will be ignored during deployment
```

## Bug reference

https://bugs.launchpad.net/juju/+bug/1984133
https://bugs.launchpad.net/juju/+bug/1983386

cmd/juju/application/bundle/bundle.go

@@ -231,28 +231,44 @@ func applicationConfigValue(key string, valueMap interface{}) (interface{}, erro
 }
 
 // ComposeAndVerifyBundle merges base and overlays then verifies the
-// combined bundle data.
-func ComposeAndVerifyBundle(base BundleDataSource, pathToOverlays []string) (*charm.BundleData, error) {
+// combined bundle data. Returns a slice of errors encountered while
+// processing the bundle. They are for informational purposes and do
+// not require failing the bundle deployment.
+func ComposeAndVerifyBundle(base BundleDataSource, pathToOverlays []string) (*charm.BundleData, []error, error) {
 	var dsList []charm.BundleDataSource
+	unMarshallErrors := make([]error, 0)
+	unMarshallErrors = append(unMarshallErrors, gatherErrors(base)...)
 
 	dsList = append(dsList, base)
 	for _, pathToOverlay := range pathToOverlays {
 		ds, err := charm.LocalBundleDataSource(pathToOverlay)
 		if err != nil {
-			return nil, errors.Annotatef(err, "unable to process overlays")
+			return nil, nil, errors.Annotatef(err, "unable to process overlays")
 		}
 		dsList = append(dsList, ds)
+		unMarshallErrors = append(unMarshallErrors, gatherErrors(ds)...)
 	}
 
 	bundleData, err := charm.ReadAndMergeBundleData(dsList...)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, nil, errors.Trace(err)
 	}
 	if err = verifyBundle(bundleData, base.BasePath()); err != nil {
-		return nil, errors.Trace(err)
+		return nil, nil, errors.Trace(err)
 	}
 
-	return bundleData, nil
+	return bundleData, unMarshallErrors, nil
+}
+
+func gatherErrors(ds BundleDataSource) []error {
+	returnErrors := make([]error, 0)
+	for _, p := range ds.Parts() {
+		if p.UnmarshallError == nil {
+			continue
+		}
+		returnErrors = append(returnErrors, p.UnmarshallError)
+	}
+	return returnErrors
 }
 
 func verifyBundle(data *charm.BundleData, bundleDir string) error {

cmd/juju/application/bundle/bundle_test.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/golang/mock/gomock"
 	"github.com/juju/charm/v8"
+	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
@@ -212,10 +213,10 @@ func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleNoOverlay(c *gc.C)
 	defer s.setupMocks(c).Finish()
 	bundleData, err := charm.ReadBundleData(strings.NewReader(wordpressBundle))
 	c.Assert(err, jc.ErrorIsNil)
-	s.expectParts(bundleData)
+	s.expectParts(&charm.BundleDataPart{Data: bundleData})
 	s.expectBasePath()
 
-	obtained, err := ComposeAndVerifyBundle(s.bundleDataSource, nil)
+	obtained, _, err := ComposeAndVerifyBundle(s.bundleDataSource, nil)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(obtained, gc.DeepEquals, bundleData)
 }
@@ -224,7 +225,7 @@ func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleOverlay(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 	bundleData, err := charm.ReadBundleData(strings.NewReader(wordpressBundle))
 	c.Assert(err, jc.ErrorIsNil)
-	s.expectParts(bundleData)
+	s.expectParts(&charm.BundleDataPart{Data: bundleData})
 	s.expectBasePath()
 	s.setupOverlayFile(c)
 
@@ -233,11 +234,35 @@ func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleOverlay(c *gc.C) {
 		"blog-title": "magic bundle config",
 	}
 
-	obtained, err := ComposeAndVerifyBundle(s.bundleDataSource, []string{s.overlayFile})
+	obtained, _, err := ComposeAndVerifyBundle(s.bundleDataSource, []string{s.overlayFile})
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(obtained, gc.DeepEquals, &expected)
 }
 
+func (s *composeAndVerifyRepSuite) TestComposeAndVerifyBundleOverlayUnmarshallErrors(c *gc.C) {
+	defer s.setupMocks(c).Finish()
+	bundleData, err := charm.ReadBundleData(strings.NewReader(typoBundle))
+	c.Assert(err, jc.ErrorIsNil)
+	expectedError := errors.New(`document 0:\n  line 1: unrecognized field "sries"\n  line 18: unrecognized field "constrai"`)
+	s.expectParts(&charm.BundleDataPart{
+		Data:            bundleData,
+		UnmarshallError: expectedError,
+	})
+	s.expectBasePath()
+	s.setupOverlayFile(c)
+
+	expected := *bundleData
+	expected.Applications["wordpress"].Options = map[string]interface{}{
+		"blog-title": "magic bundle config",
+	}
+
+	obtained, unmarshallErrors, err := ComposeAndVerifyBundle(s.bundleDataSource, []string{s.overlayFile})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(obtained, gc.DeepEquals, &expected)
+	c.Assert(unmarshallErrors, gc.HasLen, 1)
+	c.Assert(unmarshallErrors[0], gc.Equals, expectedError)
+}
+
 func (s *composeAndVerifyRepSuite) setupOverlayFile(c *gc.C) {
 	s.overlayDir = c.MkDir()
 	s.overlayFile = filepath.Join(s.overlayDir, "config.yaml")
@@ -321,9 +346,9 @@ func (s *composeAndVerifyRepSuite) setupMocks(c *gc.C) *gomock.Controller {
 	return ctrl
 }
 
-func (s *composeAndVerifyRepSuite) expectParts(bundleData *charm.BundleData) {
-	retVal := []*charm.BundleDataPart{{Data: bundleData}}
-	s.bundleDataSource.EXPECT().Parts().Return(retVal)
+func (s *composeAndVerifyRepSuite) expectParts(part *charm.BundleDataPart) {
+	retVal := []*charm.BundleDataPart{part}
+	s.bundleDataSource.EXPECT().Parts().Return(retVal).AnyTimes()
 }
 
 func (s *composeAndVerifyRepSuite) expectBasePath() {
@@ -373,3 +398,29 @@ relations:
 - - wordpress:db
   - mysql:db
 `
+
+const typoBundle = `
+sries: bionic
+applications:
+  mysql:
+    charm: cs:mysql-42
+    series: xenial
+    num_units: 1
+    to:
+    - "0"
+  wordpress:
+    charm: cs:wordpress-47
+    series: xenial
+    num_units: 1
+    to:
+    - "1"
+machines:
+  "0":
+    series: xenial
+    constrai: arch=arm64
+  "1":
+    series: xenial
+relations:
+- - wordpress:db
+  - mysql:db
+`

cmd/juju/application/deployer/bundle.go

@@ -96,10 +96,12 @@ func (d *deployBundle) deploy(
 
 	// Compose bundle to be deployed and check its validity before running
 	// any pre/post checks.
-	var bundleData *charm.BundleData
-	if bundleData, err = bundle.ComposeAndVerifyBundle(d.bundleDataSource, d.bundleOverlayFile); err != nil {
+	bundleData, unmarshalErrors, err := bundle.ComposeAndVerifyBundle(d.bundleDataSource, d.bundleOverlayFile)
+	if err != nil {
 		return errors.Annotatef(err, "cannot deploy bundle")
 	}
+	d.printDryRunUnmarshalErrors(ctx, unmarshalErrors)
+
 	d.bundleDir = d.bundleDataSource.BasePath()
 	if bundleData.UnmarshaledWithServices() {
 		logger.Warningf(`"services" key found in bundle file is deprecated, superseded by "applications" key.`)
@@ -177,6 +179,25 @@ Please repeat the deploy command with the --trust argument if you consent to tru
 	return nil
 }
 
+func (d *deployBundle) printDryRunUnmarshalErrors(ctx *cmd.Context, unmarshalErrors []error) {
+	if !d.dryRun {
+		return
+	}
+	// During a dry run, print any unmarshalling errors from the
+	// bundles and overlays
+	var msg string
+	for _, err := range unmarshalErrors {
+		if err == nil {
+			continue
+		}
+		msg = fmt.Sprintf("%s\n %s\n", msg, err)
+	}
+	if msg == "" {
+		return
+	}
+	ctx.Warningf("These fields%swill be ignored during deployment\n", msg)
+}
+
 func (d *deployBundle) makeBundleDeploySpec(ctx *cmd.Context, apiRoot DeployerAPI) (bundleDeploySpec, error) {
 	// set the consumer details API factory method on the spec, so it makes it
 	// possible to communicate with other controllers, that are found within

cmd/juju/application/diffbundle.go

@@ -209,7 +209,7 @@ func (c *diffBundleCommand) Run(ctx *cmd.Context) error {
 		return errors.Trace(err)
 	}
 
-	bundle, err := appbundle.ComposeAndVerifyBundle(baseSrc, c.bundleOverlays)
+	bundle, _, err := appbundle.ComposeAndVerifyBundle(baseSrc, c.bundleOverlays)
 	if err != nil {
 		return errors.Trace(err)
 	}

go.mod

@@ -51,7 +51,7 @@ require (
 	github.com/im7mortal/kmutex v1.0.1
 	github.com/juju/ansiterm v1.0.0
 	github.com/juju/blobstore/v2 v2.0.0
-	github.com/juju/charm/v8 v8.0.3
+	github.com/juju/charm/v8 v8.0.4
 	github.com/juju/charmrepo/v6 v6.0.2
 	github.com/juju/clock v1.0.0
 	github.com/juju/cmd/v3 v3.0.0

go.sum

@@ -498,8 +498,8 @@ github.com/juju/ansiterm v1.0.0 h1:gmMvnZRq7JZJx6jkfSq9/+2LMrVEwGwt7UR6G+lmDEg=
 github.com/juju/ansiterm v1.0.0/go.mod h1:PyXUpnI3olx3bsPcHt98FGPX/KCFZ1Fi+hw1XLI6384=
 github.com/juju/blobstore/v2 v2.0.0 h1:pYXYE7m/RL73EfuwWMQZNuG60jXlL+OsI1qVOM86nwk=
 github.com/juju/blobstore/v2 v2.0.0/go.mod h1:lFTSngYRJBy/mUOWUjAghwKcP0iXxqBYqJtuchC3cJI=
-github.com/juju/charm/v8 v8.0.3 h1:cCavlBCX6vrwd471AIjCBfskIh2WFQ5EiZ/89TXsYcE=
-github.com/juju/charm/v8 v8.0.3/go.mod h1:tZ0JfWOdv11qu4Gm5lPD0KHBeuVUH2vbrKFyYS6JUAw=
+github.com/juju/charm/v8 v8.0.4 h1:PoxtGSVIUTJW6piqPUJTXMPhVZDCchAHhg2RQnq8Sf8=
+github.com/juju/charm/v8 v8.0.4/go.mod h1:tZ0JfWOdv11qu4Gm5lPD0KHBeuVUH2vbrKFyYS6JUAw=
 github.com/juju/charmrepo/v6 v6.0.2 h1:5murmFbdrItlF292k7z3H5pY5gheOxhMNZVfdhG0tG8=
 github.com/juju/charmrepo/v6 v6.0.2/go.mod h1:7CWTdKfp00iD9XqlgNpWm1HBBLe09/vGQHwYVPqXouY=
 github.com/juju/clock v0.0.0-20190205081909-9c5c9712527c/go.mod h1:nD0vlnrUjcjJhqN5WuCWZyzfd5AHZAC9/ajvbSx69xA=