From 527de7dc2a66c08097bab59126c50cacdb579b3b Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Sat, 5 Oct 2024 16:55:51 +0200
Subject: [PATCH 01/15] feat(plan): add support for 1 sub-dir level

---
 client/plan.go               |   7 ++
 internals/cli/cmd_add.go     |   3 +
 internals/daemon/api_plan.go |   1 +
 internals/plan/plan.go       | 230 +++++++++++++++++++++++++++++++----
 4 files changed, 215 insertions(+), 26 deletions(-)

diff --git a/client/plan.go b/client/plan.go
index 25b447f08..481c7e950 100644
--- a/client/plan.go
+++ b/client/plan.go
@@ -25,6 +25,11 @@ type AddLayerOptions struct {
 	// has the given label. False (the default) means append a new layer.
 	Combine bool
 
+	// Inner true means a new layer append may go into an existing
+	// subdirectory, even through it may not result in appending it
+	// to the end of the layers slice (it becomes an insert).
+	Inner bool
+
 	// Label is the label for the new layer if appending, and the label of the
 	// layer to combine with if Combine is true.
 	Label string
@@ -38,12 +43,14 @@ func (client *Client) AddLayer(opts *AddLayerOptions) error {
 	var payload = struct {
 		Action  string `json:"action"`
 		Combine bool   `json:"combine"`
+		Inner   bool   `json:"inner"`
 		Label   string `json:"label"`
 		Format  string `json:"format"`
 		Layer   string `json:"layer"`
 	}{
 		Action:  "add",
 		Combine: opts.Combine,
+		Inner:   opts.Inner,
 		Label:   opts.Label,
 		Format:  "yaml",
 		Layer:   string(opts.LayerData),
diff --git a/internals/cli/cmd_add.go b/internals/cli/cmd_add.go
index 1d8e841cd..247a4d6c8 100644
--- a/internals/cli/cmd_add.go
+++ b/internals/cli/cmd_add.go
@@ -35,6 +35,7 @@ type cmdAdd struct {
 	client *client.Client
 
 	Combine    bool `long:"combine"`
+	Inner      bool `long:"inner"`
 	Positional struct {
 		Label     string `positional-arg-name:"<label>" required:"1"`
 		LayerPath string `positional-arg-name:"<layer-path>" required:"1"`
@@ -48,6 +49,7 @@ func init() {
 		Description: cmdAddDescription,
 		ArgsHelp: map[string]string{
 			"--combine": "Combine the new layer with an existing layer that has the given label (default is to append)",
+			"--inner":   "Allow appending a new layer inside an existing subdirectory",
 		},
 		New: func(opts *CmdOptions) flags.Commander {
 			return &cmdAdd{client: opts.Client}
@@ -65,6 +67,7 @@ func (cmd *cmdAdd) Execute(args []string) error {
 	}
 	opts := client.AddLayerOptions{
 		Combine:   cmd.Combine,
+		Inner:     cmd.Inner,
 		Label:     cmd.Positional.Label,
 		LayerData: data,
 	}
diff --git a/internals/daemon/api_plan.go b/internals/daemon/api_plan.go
index 3555b6c11..f6afef3d9 100644
--- a/internals/daemon/api_plan.go
+++ b/internals/daemon/api_plan.go
@@ -43,6 +43,7 @@ func v1PostLayers(c *Command, r *http.Request, _ *UserState) Response {
 	var payload struct {
 		Action  string `json:"action"`
 		Combine bool   `json:"combine"`
+		Inner   bool   `json:"inner"`
 		Label   string `json:"label"`
 		Format  string `json:"format"`
 		Layer   string `json:"layer"`
diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index d75146b7f..00ec9c2f3 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -1227,7 +1227,29 @@ func (p *Plan) checkCycles() error {
 	return err
 }
 
+// labelExp represents a match of a valid layer label, which may include a
+// directory prefix (which excludes the '.d' ending, in the same way the
+// file suffix is omitted).
+//
+// ┌─────────┬─────────────────────────────────────┐
+// │ Label   │ Description                         │
+// ├─────────┼─────────────────────────────────────┤
+// │         │                                     │
+// │ abc     │ Label of file in layers root        │
+// │         │                                     │
+// │ foo/bar │ Label of file inside sub-directory  │
+// │         │                                     │
+// └─────────┴─────────────────────────────────────┘
+var labelExp = regexp.MustCompile(`^(([a-z](?:-?[a-z0-9]){2,})/)?([a-z](?:-?[a-z0-9]){2,})$`)
+
 func ParseLayer(order int, label string, data []byte) (*Layer, error) {
+	// This function can be called directly over the daemon API. We
+	// must fail the API request if the label is not valid.
+	match := labelExp.FindStringSubmatch(label)
+	if match == nil {
+		return nil, fmt.Errorf("cannot parse layer: invalid label %q", label)
+	}
+
 	layer := &Layer{
 		Services:   make(map[string]*Service),
 		Checks:     make(map[string]*Check),
@@ -1349,44 +1371,203 @@ func validServiceAction(action ServiceAction, additionalValid ...ServiceAction)
 	}
 }
 
-var fnameExp = regexp.MustCompile("^([0-9]{3})-([a-z](?:-?[a-z0-9]){2,}).yaml$")
-
-func ReadLayersDir(dirname string) ([]*Layer, error) {
-	finfos, err := os.ReadDir(dirname)
+// configLayerExp represents a match for a valid layer relative path, which
+// typically consists of only the file name ending with '.yaml'. Optionally,
+// it may include a single level subdirectory prefix which ends with '.d'.
+//
+// ┌─────────────────────────────────────────────────────────────┐
+// │ Match Index                                                 │
+// ├────────────────────────┬────────────┬─────┬─────┬─────┬─────┤
+// │ 0 (Supplied Path)      │ 1          │ 2   │ 3   │ 4   │ 5   │
+// ├────────────────────────┼────────────┼─────┼─────┼─────┼─────┤
+// │                        │            │     │     │     │     │
+// │ 001-abc.yaml           │            │     │     │ 001 │ abc │
+// │                        │            │     │     │     │     │
+// │ 002-foo.d/001-bar.yaml │ 002-foo.d/ │ 002 │ foo │ 001 │ bar │
+// │                        │            │     │     │     │     │
+// └────────────────────────┴────────────┴─────┴─────┴─────┴─────┘
+var layerPathExp = regexp.MustCompile(`^(([0-9]{3})-([a-z](?:-?[a-z0-9]){2,})\.d/)?([0-9]{3})-([a-z](?:-?[a-z0-9]){2,}).yaml$`)
+
+// ReadLayersDir loads the YAML layer files from the first two directory
+// levels starting at layersDir in the order as specified by the order
+// directory and file prefix (see configLayersTwoLevels). Note that the
+// directory and file suffix is dropped for the label.
+//
+// ┌────────────────────────┬──────────────────┬─────────┐
+// │ Layer Path             │ Order            │ Label   │
+// ├────────────────────────┼──────────────────┼─────────┤
+// │                        │                  │         │
+// │ 001-abc.yaml           │ 001-000 => 1000  │ abc     │
+// │                        │                  │         │
+// │ 002-foo.d/001-bar.yaml │ 002-001 => 2001  │ foo/bar │
+// │                        │                  │         │
+// └────────────────────────┴──────────────────┴─────────┘
+func ReadLayersDir(layersDir string) ([]*Layer, error) {
+	orderedConfigLayers, err := configLayersTwoLevels(layersDir)
 	if err != nil {
 		// Errors from package os generally include the path.
-		return nil, fmt.Errorf("cannot read layers directory: %v", err)
+		return nil, err
 	}
 
-	orders := make(map[int]string)
-	labels := make(map[string]int)
-
-	// Documentation says ReadDir result is already sorted by name.
-	// This is fundamental here so if reading changes make sure the
-	// sorting is preserved.
 	var layers []*Layer
-	for _, finfo := range finfos {
-		if finfo.IsDir() || !strings.HasSuffix(finfo.Name(), ".yaml") {
-			continue
-		}
+	for _, configLayerPath := range orderedConfigLayers {
 		// TODO Consider enforcing permissions and ownership here to
 		//      avoid mistakes that could lead to hacks.
-		match := fnameExp.FindStringSubmatch(finfo.Name())
+		match := layerPathExp.FindStringSubmatch(configLayerPath)
 		if match == nil {
-			return nil, fmt.Errorf("invalid layer filename: %q (must look like \"123-some-label.yaml\")", finfo.Name())
+			return nil, fmt.Errorf("internal error: failed to parse layer path %q", configLayerPath)
+		}
+
+		// Resolve the order and label, which may include additional
+		// information from an optional sub-directory prefix.
+		label := filepath.Join(match[3], match[5])
+		var orderStr string
+		if match[1] == "" {
+			// Config layer is the layers root.
+			orderStr = match[4] + "000"
+		} else {
+			// Config layer is inside a sub-directory.
+			orderStr = match[2] + match[4]
+		}
+		order, err := strconv.Atoi(orderStr)
+		if err != nil {
+			panic(fmt.Sprintf("internal error: filename regexp is wrong: %v", err))
 		}
 
-		data, err := os.ReadFile(filepath.Join(dirname, finfo.Name()))
+		data, err := os.ReadFile(filepath.Join(layersDir, configLayerPath))
 		if err != nil {
 			// Errors from package os generally include the path.
 			return nil, fmt.Errorf("cannot read layer file: %v", err)
 		}
+
+		layer, err := ParseLayer(order, label, data)
+		if err != nil {
+			return nil, err
+		}
+		layers = append(layers, layer)
+	}
+	return layers, nil
+}
+
+// configLayersTwoLevels locates all layer configuration files in the first
+// two directory levels starting in layersDir. If the first level contains
+// a directory symlink the second level files will be located by following
+// the link. The function returns an ordered list of configuration file names
+// as shown in the example below.
+//
+// ┌────────────────────────────┐
+// │ File (ordered)             │
+// ├────────────────────────────┤
+// │                            │
+// │ 001-foo.yaml               │
+// │                            │
+// │ 002-bar.d/001-aaa.yaml     │
+// │                            │
+// │ 002-bar.d/002-bbb.yaml     │
+// │                            │
+// │ 003-baz.yaml               │
+// │                            │
+// └────────────────────────────┘
+func configLayersTwoLevels(layersDir string) (relPath []string, err error) {
+	// Read the first-level directory
+	l1Entries, err := configLayerEntries(layersDir, true)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, l1Entry := range l1Entries {
+		l1Path := filepath.Join(layersDir, l1Entry)
+
+		// Let's check if the path (including a symlink) is a directory.
+		info, err := os.Stat(l1Path)
+		if err != nil {
+			return nil, err
+		}
+
+		if info.IsDir() {
+			// Read the second-level directory
+			l2Entries, err := configLayerEntries(l1Path, false)
+			if err != nil {
+				return nil, err
+			}
+
+			// Add the config files from the second level
+			for _, l2Entry := range l2Entries {
+				relPath = append(relPath, filepath.Join(l1Entry, l2Entry))
+			}
+		} else {
+			// Add the config files from the first level
+			relPath = append(relPath, l1Entry)
+		}
+	}
+	return relPath, nil
+}
+
+// configEntryExp matches either a valid config layer YAML file name or it
+// matches a valid config layer directory.
+//
+// ┌────────────────────────────────────────────────────────┐
+// │ Match Index                                            │
+// ├────────────────────────┬───────────┬───────────┬───────┤
+// │ 0 (Supplied Name)      │ 1 (order) │ 2 (label) │ 3     │
+// ├────────────────────────┼───────────┼───────────┼───────┤
+// │                        │           │           │       │
+// │ 001-abc.yaml           │ 001       │ abc       │ .yaml │
+// │                        │           │           │       │
+// │ 002-foo.d              │ 002       │ foo       │ .d    │
+// │                        │           │           │       │
+// └────────────────────────┴───────────┴───────────┴───────┘
+var configEntryExp = regexp.MustCompile(`^([0-9]{3})-([a-z](?:-?[a-z0-9]){2,})(.yaml|.d)$`)
+
+// configLayerEntries reads a directory containing config layer files or
+// sub-directories and validates that the naming is valid. If dirOK is
+// set to false it will not permit sub-directories within the supplied
+// configDir path. The returned string slice is ordered alphanumerically
+// (so names are automatically ordered by their 'order').
+func configLayerEntries(configDir string, dirOK bool) (names []string, err error) {
+	entries, err := os.ReadDir(configDir)
+	if err != nil {
+		return nil, fmt.Errorf("cannot read layers directory: %v", err)
+	}
+
+	orders := make(map[int]string)
+	labels := make(map[string]int)
+	for _, entry := range entries {
+		// Let's not fail to start the system up if some unrelated file ended
+		// up in the layers directory by accident.
+		if !strings.HasSuffix(entry.Name(), ".yaml") && !strings.HasSuffix(entry.Name(), ".d") {
+			continue
+		}
+
+		// Let's check if the path (including a symlink) is a directory.
+		info, err := os.Stat(filepath.Join(configDir, entry.Name()))
+		if err != nil {
+			return nil, err
+		}
+
+		// Let's ensure the file or sub-directory name is valid.
+		match := configEntryExp.FindStringSubmatch(entry.Name())
+		if match == nil {
+			if info.IsDir() {
+				return nil, fmt.Errorf("invalid layer sub-directory name: %q (must look like \"123-some-label.d\")", entry.Name())
+			} else {
+				return nil, fmt.Errorf("invalid layer filename: %q (must look like \"123-some-label.yaml\")", entry.Name())
+			}
+		}
+
+		// Only the root layers directory support sub-directories.
+		if info.IsDir() && !dirOK {
+			return nil, fmt.Errorf("cannot have a layers sub-directory at this level")
+		}
+
+		// Extract the order and label from the match.
 		label := match[2]
 		order, err := strconv.Atoi(match[1])
 		if err != nil {
 			panic(fmt.Sprintf("internal error: filename regexp is wrong: %v", err))
 		}
 
+		// Let's make sure no duplicate orders or labels appear.
 		oldLabel, dupOrder := orders[order]
 		oldOrder, dupLabel := labels[label]
 		if dupOrder {
@@ -1395,19 +1576,16 @@ func ReadLayersDir(dirname string) ([]*Layer, error) {
 			oldLabel = label
 		}
 		if dupOrder || dupLabel {
-			return nil, fmt.Errorf("invalid layer filename: %q not unique (have \"%03d-%s.yaml\" already)", finfo.Name(), oldOrder, oldLabel)
+			return nil, fmt.Errorf("invalid layer filename: %q not unique (have \"%03d-%s.yaml\" already)", entry.Name(), oldOrder, oldLabel)
 		}
-
 		orders[order] = label
 		labels[label] = order
 
-		layer, err := ParseLayer(order, label, data)
-		if err != nil {
-			return nil, err
-		}
-		layers = append(layers, layer)
+		// All is good for this entry.
+		names = append(names, entry.Name())
 	}
-	return layers, nil
+
+	return names, nil
 }
 
 // ReadDir reads the configuration layers from layersDir,

From 95006b8596ebcaeb11290bf8fc87ce4f88361145 Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 14 Oct 2024 12:36:00 +0200
Subject: [PATCH 02/15] Review feedback group 1

---
 client/plan.go         |   2 +-
 internals/plan/plan.go | 150 +++++++++++++++++------------------------
 2 files changed, 61 insertions(+), 91 deletions(-)

diff --git a/client/plan.go b/client/plan.go
index 481c7e950..8add24f0b 100644
--- a/client/plan.go
+++ b/client/plan.go
@@ -26,7 +26,7 @@ type AddLayerOptions struct {
 	Combine bool
 
 	// Inner true means a new layer append may go into an existing
-	// subdirectory, even through it may not result in appending it
+	// subdirectory, even though it may not result in appending it
 	// to the end of the layers slice (it becomes an insert).
 	Inner bool
 
diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index 00ec9c2f3..f8646defb 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -1371,27 +1371,10 @@ func validServiceAction(action ServiceAction, additionalValid ...ServiceAction)
 	}
 }
 
-// configLayerExp represents a match for a valid layer relative path, which
-// typically consists of only the file name ending with '.yaml'. Optionally,
-// it may include a single level subdirectory prefix which ends with '.d'.
-//
-// ┌─────────────────────────────────────────────────────────────┐
-// │ Match Index                                                 │
-// ├────────────────────────┬────────────┬─────┬─────┬─────┬─────┤
-// │ 0 (Supplied Path)      │ 1          │ 2   │ 3   │ 4   │ 5   │
-// ├────────────────────────┼────────────┼─────┼─────┼─────┼─────┤
-// │                        │            │     │     │     │     │
-// │ 001-abc.yaml           │            │     │     │ 001 │ abc │
-// │                        │            │     │     │     │     │
-// │ 002-foo.d/001-bar.yaml │ 002-foo.d/ │ 002 │ foo │ 001 │ bar │
-// │                        │            │     │     │     │     │
-// └────────────────────────┴────────────┴─────┴─────┴─────┴─────┘
-var layerPathExp = regexp.MustCompile(`^(([0-9]{3})-([a-z](?:-?[a-z0-9]){2,})\.d/)?([0-9]{3})-([a-z](?:-?[a-z0-9]){2,}).yaml$`)
-
 // ReadLayersDir loads the YAML layer files from the first two directory
 // levels starting at layersDir in the order as specified by the order
-// directory and file prefix (see configLayersTwoLevels). Note that the
-// directory and file suffix is dropped for the label.
+// directory and file prefixes. Note that the directory and file suffix
+// is dropped for the label.
 //
 // ┌────────────────────────┬──────────────────┬─────────┐
 // │ Layer Path             │ Order            │ Label   │
@@ -1403,72 +1386,8 @@ var layerPathExp = regexp.MustCompile(`^(([0-9]{3})-([a-z](?:-?[a-z0-9]){2,})\.d
 // │                        │                  │         │
 // └────────────────────────┴──────────────────┴─────────┘
 func ReadLayersDir(layersDir string) ([]*Layer, error) {
-	orderedConfigLayers, err := configLayersTwoLevels(layersDir)
-	if err != nil {
-		// Errors from package os generally include the path.
-		return nil, err
-	}
-
 	var layers []*Layer
-	for _, configLayerPath := range orderedConfigLayers {
-		// TODO Consider enforcing permissions and ownership here to
-		//      avoid mistakes that could lead to hacks.
-		match := layerPathExp.FindStringSubmatch(configLayerPath)
-		if match == nil {
-			return nil, fmt.Errorf("internal error: failed to parse layer path %q", configLayerPath)
-		}
 
-		// Resolve the order and label, which may include additional
-		// information from an optional sub-directory prefix.
-		label := filepath.Join(match[3], match[5])
-		var orderStr string
-		if match[1] == "" {
-			// Config layer is the layers root.
-			orderStr = match[4] + "000"
-		} else {
-			// Config layer is inside a sub-directory.
-			orderStr = match[2] + match[4]
-		}
-		order, err := strconv.Atoi(orderStr)
-		if err != nil {
-			panic(fmt.Sprintf("internal error: filename regexp is wrong: %v", err))
-		}
-
-		data, err := os.ReadFile(filepath.Join(layersDir, configLayerPath))
-		if err != nil {
-			// Errors from package os generally include the path.
-			return nil, fmt.Errorf("cannot read layer file: %v", err)
-		}
-
-		layer, err := ParseLayer(order, label, data)
-		if err != nil {
-			return nil, err
-		}
-		layers = append(layers, layer)
-	}
-	return layers, nil
-}
-
-// configLayersTwoLevels locates all layer configuration files in the first
-// two directory levels starting in layersDir. If the first level contains
-// a directory symlink the second level files will be located by following
-// the link. The function returns an ordered list of configuration file names
-// as shown in the example below.
-//
-// ┌────────────────────────────┐
-// │ File (ordered)             │
-// ├────────────────────────────┤
-// │                            │
-// │ 001-foo.yaml               │
-// │                            │
-// │ 002-bar.d/001-aaa.yaml     │
-// │                            │
-// │ 002-bar.d/002-bbb.yaml     │
-// │                            │
-// │ 003-baz.yaml               │
-// │                            │
-// └────────────────────────────┘
-func configLayersTwoLevels(layersDir string) (relPath []string, err error) {
 	// Read the first-level directory
 	l1Entries, err := configLayerEntries(layersDir, true)
 	if err != nil {
@@ -1476,7 +1395,7 @@ func configLayersTwoLevels(layersDir string) (relPath []string, err error) {
 	}
 
 	for _, l1Entry := range l1Entries {
-		l1Path := filepath.Join(layersDir, l1Entry)
+		l1Path := filepath.Join(layersDir, l1Entry.name)
 
 		// Let's check if the path (including a symlink) is a directory.
 		info, err := os.Stat(l1Path)
@@ -1493,14 +1412,55 @@ func configLayersTwoLevels(layersDir string) (relPath []string, err error) {
 
 			// Add the config files from the second level
 			for _, l2Entry := range l2Entries {
-				relPath = append(relPath, filepath.Join(l1Entry, l2Entry))
+				layer, err := configLayerLoad(layersDir, l1Entry, l2Entry)
+				if err != nil {
+					return nil, err
+				}
+				layers = append(layers, layer)
 			}
 		} else {
 			// Add the config files from the first level
-			relPath = append(relPath, l1Entry)
+			layer, err := configLayerLoad(layersDir, l1Entry, nil)
+			if err != nil {
+				return nil, err
+			}
+			layers = append(layers, layer)
 		}
 	}
-	return relPath, nil
+
+	return layers, nil
+}
+
+// configLayerLoad loads a layer configuration file and returns a Layer
+// on success. The layer configuration is typically in the Pebble layers
+// root directory, in which case l2Entry must be nil. If the file is
+// inside a sub-directory, l1Entry must supply information on the directory,
+// while l2Entry information on the file name itself.
+func configLayerLoad(layersDir string, l1Entry *configEntry, l2Entry *configEntry) (*Layer, error) {
+	// Resolve the order and label, which may include additional
+	// information from an optional sub-directory prefix.
+	label := l1Entry.label
+	order := 1000 * l1Entry.order
+	path := filepath.Join(layersDir, l1Entry.name)
+	if l2Entry != nil {
+		// Config layer is inside a sub-directory.
+		label = filepath.Join(label, l2Entry.label)
+		order = order + l2Entry.order
+		path = filepath.Join(path, l2Entry.name)
+	}
+
+	data, err := os.ReadFile(path)
+	if err != nil {
+		// Errors from package os generally include the path.
+		return nil, fmt.Errorf("cannot read layer file: %v", err)
+	}
+
+	layer, err := ParseLayer(order, label, data)
+	if err != nil {
+		return nil, err
+	}
+
+	return layer, nil
 }
 
 // configEntryExp matches either a valid config layer YAML file name or it
@@ -1519,12 +1479,18 @@ func configLayersTwoLevels(layersDir string) (relPath []string, err error) {
 // └────────────────────────┴───────────┴───────────┴───────┘
 var configEntryExp = regexp.MustCompile(`^([0-9]{3})-([a-z](?:-?[a-z0-9]){2,})(.yaml|.d)$`)
 
+type configEntry struct {
+	name  string
+	order int
+	label string
+}
+
 // configLayerEntries reads a directory containing config layer files or
 // sub-directories and validates that the naming is valid. If dirOK is
 // set to false it will not permit sub-directories within the supplied
 // configDir path. The returned string slice is ordered alphanumerically
 // (so names are automatically ordered by their 'order').
-func configLayerEntries(configDir string, dirOK bool) (names []string, err error) {
+func configLayerEntries(configDir string, dirOK bool) (configs []*configEntry, err error) {
 	entries, err := os.ReadDir(configDir)
 	if err != nil {
 		return nil, fmt.Errorf("cannot read layers directory: %v", err)
@@ -1582,10 +1548,14 @@ func configLayerEntries(configDir string, dirOK bool) (names []string, err error
 		labels[label] = order
 
 		// All is good for this entry.
-		names = append(names, entry.Name())
+		configs = append(configs, &configEntry{
+			name:  entry.Name(),
+			order: order,
+			label: label,
+		})
 	}
 
-	return names, nil
+	return configs, nil
 }
 
 // ReadDir reads the configuration layers from layersDir,

From 234045dc91f38dc7a8a569cfeacf1a6343381a78 Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 14 Oct 2024 13:58:19 +0200
Subject: [PATCH 03/15] Fix unit tests

---
 client/plan_test.go                          | 1 +
 internals/cli/cmd_add_test.go                | 1 +
 internals/overlord/servstate/manager_test.go | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/client/plan_test.go b/client/plan_test.go
index 385d6e202..8e55a5f32 100644
--- a/client/plan_test.go
+++ b/client/plan_test.go
@@ -53,6 +53,7 @@ services:
 			"label":   "foo",
 			"format":  "yaml",
 			"layer":   layerYAML,
+			"inner":   false,
 		})
 	}
 }
diff --git a/internals/cli/cmd_add_test.go b/internals/cli/cmd_add_test.go
index a9990c581..c037be6fb 100644
--- a/internals/cli/cmd_add_test.go
+++ b/internals/cli/cmd_add_test.go
@@ -58,6 +58,7 @@ services:
 					"label":   "foo",
 					"format":  "yaml",
 					"layer":   layerYAML,
+					"inner":   false,
 				})
 				fmt.Fprint(w, `{
     "type": "sync",
diff --git a/internals/overlord/servstate/manager_test.go b/internals/overlord/servstate/manager_test.go
index c097336ca..5a7faf4a3 100644
--- a/internals/overlord/servstate/manager_test.go
+++ b/internals/overlord/servstate/manager_test.go
@@ -1707,7 +1707,7 @@ func (s *S) planChanged(c *C) {
 
 func (s *S) planAddLayer(c *C, layerYAML string) {
 	cnt := len(s.plan.Layers)
-	layer, err := plan.ParseLayer(cnt, fmt.Sprintf("testPlanLayer%v", cnt), []byte(layerYAML))
+	layer, err := plan.ParseLayer(cnt, fmt.Sprintf("test-plan-layer-%v", cnt), []byte(layerYAML))
 	c.Assert(err, IsNil)
 	// Resolve {{.NotifyDoneCheck}}
 	s.insertDoneChecks(c, layer)

From b4db34fc3aafee563be481609c22366b7d9dda0a Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 14 Oct 2024 14:03:07 +0200
Subject: [PATCH 04/15] fix docs

---
 docs/reference/cli-commands/add.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/docs/reference/cli-commands/add.md b/docs/reference/cli-commands/add.md
index 5ef741108..9e2299799 100644
--- a/docs/reference/cli-commands/add.md
+++ b/docs/reference/cli-commands/add.md
@@ -19,5 +19,7 @@ label (or append if the label is not found).
 [add command options]
       --combine         Combine the new layer with an existing layer that has
                         the given label (default is to append)
+      --inner           Allow appending a new layer inside an existing
+                        subdirectory
 ```
 <!-- END AUTOMATED OUTPUT -->

From 19e9c9e70eb49f46877ccf0f1649707c0b06d52a Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 14 Oct 2024 21:33:09 +0200
Subject: [PATCH 05/15] Fix all unit tests

---
 internals/daemon/api_exec_test.go            |  4 +-
 internals/daemon/api_plan.go                 |  4 +-
 internals/overlord/planstate/manager.go      | 75 ++++++++++++++++----
 internals/overlord/planstate/manager_test.go | 42 +++++------
 4 files changed, 86 insertions(+), 39 deletions(-)

diff --git a/internals/daemon/api_exec_test.go b/internals/daemon/api_exec_test.go
index 37694882b..b0ec05528 100644
--- a/internals/daemon/api_exec_test.go
+++ b/internals/daemon/api_exec_test.go
@@ -206,7 +206,7 @@ func (s *execSuite) TestContextNoOverrides(c *C) {
 			Environment: map[string]string{"FOO": "foo", "BAR": "bar"},
 			WorkingDir:  dir,
 		}},
-	})
+	}, false)
 	c.Assert(err, IsNil)
 
 	stdout, stderr, err := s.exec(c, "", &client.ExecOptions{
@@ -228,7 +228,7 @@ func (s *execSuite) TestContextOverrides(c *C) {
 			Environment: map[string]string{"FOO": "foo", "BAR": "bar"},
 			WorkingDir:  c.MkDir(),
 		}},
-	})
+	}, false)
 	c.Assert(err, IsNil)
 
 	overrideDir := c.MkDir()
diff --git a/internals/daemon/api_plan.go b/internals/daemon/api_plan.go
index f6afef3d9..1ad5e4fec 100644
--- a/internals/daemon/api_plan.go
+++ b/internals/daemon/api_plan.go
@@ -69,9 +69,9 @@ func v1PostLayers(c *Command, r *http.Request, _ *UserState) Response {
 
 	planMgr := overlordPlanManager(c.d.overlord)
 	if payload.Combine {
-		err = planMgr.CombineLayer(layer)
+		err = planMgr.CombineLayer(layer, payload.Inner)
 	} else {
-		err = planMgr.AppendLayer(layer)
+		err = planMgr.AppendLayer(layer, payload.Inner)
 	}
 	if err != nil {
 		if _, ok := err.(*planstate.LabelExists); ok {
diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go
index a4fb44140..a42650bd1 100644
--- a/internals/overlord/planstate/manager.go
+++ b/internals/overlord/planstate/manager.go
@@ -16,6 +16,8 @@ package planstate
 
 import (
 	"fmt"
+	"slices"
+	"strings"
 	"sync"
 
 	"github.com/canonical/pebble/internals/plan"
@@ -100,8 +102,10 @@ func (m *PlanManager) Plan() *plan.Plan {
 
 // AppendLayer takes a Layer, appends it to the plan's layers and updates the
 // layer.Order field to the new order. If a layer with layer.Label already
-// exists, return an error of type *LabelExists.
-func (m *PlanManager) AppendLayer(layer *plan.Layer) error {
+// exists, return an error of type *LabelExists. Inner must be set to true
+// if the append operation may be demoted to an insert due to the layer
+// configuration being located in a sub-directory.
+func (m *PlanManager) AppendLayer(layer *plan.Layer, inner bool) error {
 	var newPlan *plan.Plan
 	defer func() { m.callChangeListeners(newPlan) }()
 
@@ -113,14 +117,17 @@ func (m *PlanManager) AppendLayer(layer *plan.Layer) error {
 		return &LabelExists{Label: layer.Label}
 	}
 
-	newPlan, err := m.appendLayer(layer)
+	newPlan, err := m.appendLayer(layer, inner)
 	return err
 }
 
 // CombineLayer takes a Layer, combines it to an existing layer that has the
 // same label. If no existing layer has the label, append a new one. In either
-// case, update the layer.Order field to the new order.
-func (m *PlanManager) CombineLayer(layer *plan.Layer) error {
+// case, update the layer.Order field to the new order. Inner must be set to
+// true if a combine operation gets demoted to an append operation (due to the
+// layer not yet existing), and if the configuration layer is located in a
+// sub-directory (see AppendLayer).
+func (m *PlanManager) CombineLayer(layer *plan.Layer, inner bool) error {
 	var newPlan *plan.Plan
 	defer func() { m.callChangeListeners(newPlan) }()
 
@@ -131,7 +138,7 @@ func (m *PlanManager) CombineLayer(layer *plan.Layer) error {
 	if index < 0 {
 		// No layer found with this label, append new one.
 		var err error
-		newPlan, err = m.appendLayer(layer)
+		newPlan, err = m.appendLayer(layer, inner)
 		return err
 	}
 
@@ -155,19 +162,59 @@ func (m *PlanManager) CombineLayer(layer *plan.Layer) error {
 	return nil
 }
 
-func (m *PlanManager) appendLayer(layer *plan.Layer) (*plan.Plan, error) {
-	newOrder := 1
-	if len(m.plan.Layers) > 0 {
-		last := m.plan.Layers[len(m.plan.Layers)-1]
-		newOrder = last.Order + 1
+func (m *PlanManager) appendLayer(newLayer *plan.Layer, inner bool) (*plan.Plan, error) {
+	// The starting index and order assumes no existing layers.
+	newIndex := 0
+	newOrder := 1000
+
+	// If we have existing layers, things get a little bit more complex.
+	layersCount := len(m.plan.Layers)
+	if layersCount > 0 {
+		// We know at this point the complete label does not yet exist.
+		// However, let's see if the first part of the label is a
+		// sub-directory that exists and for which we already allocated
+		// an order?
+		newSubLabel, _, hasSub := strings.Cut(newLayer.Label, "/")
+		lastIndex := layersCount - 1
+		for i, _ := range m.plan.Layers {
+			layer := m.plan.Layers[lastIndex-i]
+			layerSubLabel, _, _ := strings.Cut(layer.Label, "/")
+			// If we have a sub-directory match we know it already exists.
+			// Since we searched backwards, we know the order should be the
+			// next integer value.
+			if layerSubLabel == newSubLabel {
+				newOrder = layer.Order + 1
+				newIndex = lastIndex - i + 1
+			}
+		}
+
+		// If we did not match a sub-directory, this is simply an append.
+		// However, we need to know if this a inside a sub-directory or not
+		// as it has an impact on how we allocate the order.
+		if newIndex == 0 {
+			newIndex = layersCount
+			newOrder = ((m.plan.Layers[layersCount-1].Order / 1000) + 1) * 1000
+			if hasSub {
+				// The file in the sub-directory needs an order to "001".
+				newOrder += 1
+			}
+		}
+	}
+
+	// If the append operation requires an insert because the layer is added
+	// inside an already existing sub-directory, with higher order items already
+	// allocated beyond it, we allow it only if the request specifically
+	// authorised it (inner=true).
+	if newIndex != layersCount && !inner {
+		return nil, fmt.Errorf("cannot insert sub-directory layer without 'inner' attribute set")
 	}
 
-	newLayers := append(m.plan.Layers, layer)
+	newLayers := slices.Insert(m.plan.Layers, newIndex, newLayer)
 	newPlan, err := m.updatePlanLayers(newLayers)
 	if err != nil {
 		return nil, err
 	}
-	layer.Order = newOrder
+	newLayer.Order = newOrder
 	return newPlan, nil
 }
 
@@ -243,6 +290,6 @@ func (m *PlanManager) SetServiceArgs(serviceArgs map[string][]string) error {
 		}
 	}
 
-	newPlan, err := m.appendLayer(newLayer)
+	newPlan, err := m.appendLayer(newLayer, false)
 	return err
 }
diff --git a/internals/overlord/planstate/manager_test.go b/internals/overlord/planstate/manager_test.go
index 300e04113..25cf34cce 100644
--- a/internals/overlord/planstate/manager_test.go
+++ b/internals/overlord/planstate/manager_test.go
@@ -116,9 +116,9 @@ test-field:
         override: replace
         a: something
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 1)
+	c.Assert(layer.Order, Equals, 1000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -142,7 +142,7 @@ test-field:
         override: foobar
         a: something else
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
 	c.Assert(err.(*planstate.LabelExists).Label, Equals, "label1")
 	c.Assert(ps.planYAML(c), Equals, `
 services:
@@ -167,9 +167,9 @@ test-field:
         override: replace
         a: else
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 2)
+	c.Assert(layer.Order, Equals, 2000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -193,9 +193,9 @@ test-field:
         override: replace
         a: something
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 3)
+	c.Assert(layer.Order, Equals, 3000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -233,9 +233,9 @@ test-field:
         override: replace
         a: something
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 1)
+	c.Assert(layer.Order, Equals, 1000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -259,9 +259,9 @@ test-field:
         override: replace
         a: else
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 2)
+	c.Assert(layer.Order, Equals, 2000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -291,9 +291,9 @@ test-field:
         override: replace
         a: else
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 1)
+	c.Assert(layer.Order, Equals, 1000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -323,9 +323,9 @@ test-field:
         override: replace
         a: something
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 2)
+	c.Assert(layer.Order, Equals, 2000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -361,9 +361,9 @@ test-field:
         override: replace
         a: nothing
 `)
-	err = ps.planMgr.CombineLayer(layer)
+	err = ps.planMgr.CombineLayer(layer, false)
 	c.Assert(err, IsNil)
-	c.Assert(layer.Order, Equals, 3)
+	c.Assert(layer.Order, Equals, 3000)
 	c.Assert(ps.planYAML(c), Equals, `
 services:
     svc1:
@@ -421,7 +421,7 @@ services:
         override: replace
         command: foo
 `)
-	err = ps.planMgr.AppendLayer(layer)
+	err = ps.planMgr.AppendLayer(layer, false)
 
 	// Set arguments to services.
 	serviceArgs := map[string][]string{
@@ -477,10 +477,10 @@ services:
         override: replace
         command: /bin/sh
 `)
-		err = manager.AppendLayer(layer1)
+		err = manager.AppendLayer(layer1, false)
 		c.Assert(err, IsNil)
 
-		err = manager.CombineLayer(layer1)
+		err = manager.CombineLayer(layer1, false)
 		c.Assert(err, IsNil)
 
 		layer2 := ps.parseLayer(c, 0, "label2", `
@@ -489,7 +489,7 @@ services:
         override: replace
         command: /bin/sh
 `)
-		err = manager.CombineLayer(layer2)
+		err = manager.CombineLayer(layer2, false)
 		c.Assert(err, IsNil)
 
 		err = manager.SetServiceArgs(map[string][]string{

From f2dfaab39600f16e29749483c33f93463934acb5 Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 14 Oct 2024 21:40:37 +0200
Subject: [PATCH 06/15] staticcheck plan

---
 internals/plan/extensions_test.go | 18 +++++++-----------
 internals/plan/plan.go            |  8 ++++----
 internals/plan/plan_test.go       |  1 +
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go
index 0be9d0518..ff193b1c9 100644
--- a/internals/plan/extensions_test.go
+++ b/internals/plan/extensions_test.go
@@ -16,7 +16,6 @@ package plan_test
 
 import (
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"slices"
@@ -402,8 +401,7 @@ nexttest:
 			return n.field == xField
 		}) {
 			// Verify "x-field" data.
-			var x *xSection
-			x = p.Sections[xField].(*xSection)
+			x := p.Sections[xField].(*xSection)
 			c.Assert(err, IsNil)
 			c.Assert(x.Entries, DeepEquals, testData.result.x.Entries)
 		}
@@ -412,8 +410,7 @@ nexttest:
 			return n.field == yField
 		}) {
 			// Verify "y-field" data.
-			var y *ySection
-			y = p.Sections[yField].(*ySection)
+			y := p.Sections[yField].(*ySection)
 			c.Assert(err, IsNil)
 			c.Assert(y.Entries, DeepEquals, testData.result.y.Entries)
 		}
@@ -474,6 +471,7 @@ func (s *S) TestSectionOrderExt(c *C) {
 		Sections:   combined.Sections,
 	}
 	data, err := yaml.Marshal(plan)
+	c.Assert(err, IsNil)
 	c.Assert(string(data), Equals, string(reindent(`
 		services:
 			srv1:
@@ -511,7 +509,7 @@ func (s *S) writeLayerFiles(c *C, layersDir string, inputs []*inputLayer) {
 	c.Assert(err, IsNil)
 
 	for _, input := range inputs {
-		err := ioutil.WriteFile(filepath.Join(layersDir, fmt.Sprintf("%03d-%s.yaml", input.order, input.label)), reindent(input.yaml), 0644)
+		err := os.WriteFile(filepath.Join(layersDir, fmt.Sprintf("%03d-%s.yaml", input.order, input.label)), reindent(input.yaml), 0644)
 		c.Assert(err, IsNil)
 	}
 }
@@ -548,17 +546,15 @@ func (x xExtension) CombineSections(sections ...plan.Section) (plan.Section, err
 }
 
 func (x xExtension) ValidatePlan(p *plan.Plan) error {
-	var xs *xSection
-	xs = p.Sections[xField].(*xSection)
+	xs := p.Sections[xField].(*xSection)
 	if xs != nil {
-		var ys *ySection
-		ys = p.Sections[yField].(*ySection)
+		ys := p.Sections[yField].(*ySection)
 
 		// Test dependency: Make sure every Y field in X refer to an existing Y entry.
 		for xEntryField, xEntryValue := range xs.Entries {
 			for _, yReference := range xEntryValue.Y {
 				found := false
-				for yEntryField, _ := range ys.Entries {
+				for yEntryField := range ys.Entries {
 					if yReference == yEntryField {
 						found = true
 						break
diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index f8646defb..c71698332 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -823,7 +823,7 @@ func (layer *Layer) Validate() error {
 	for name, service := range layer.Services {
 		if name == "" {
 			return &FormatError{
-				Message: fmt.Sprintf("cannot use empty string as service name"),
+				Message: "cannot use empty string as service name",
 			}
 		}
 		if name == "pebble" {
@@ -880,7 +880,7 @@ func (layer *Layer) Validate() error {
 	for name, check := range layer.Checks {
 		if name == "" {
 			return &FormatError{
-				Message: fmt.Sprintf("cannot use empty string as check name"),
+				Message: "cannot use empty string as check name",
 			}
 		}
 		if check == nil {
@@ -890,7 +890,7 @@ func (layer *Layer) Validate() error {
 		}
 		if name == "" {
 			return &FormatError{
-				Message: fmt.Sprintf("cannot use empty string as log target name"),
+				Message: "cannot use empty string as log target name",
 			}
 		}
 		if check.Level != UnsetLevel && check.Level != AliveLevel && check.Level != ReadyLevel {
@@ -1281,7 +1281,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) {
 	// will at least have an empty node. This means we can consistently
 	// let the extension allocate and decode the yaml node for all sections,
 	// and in the case where it is zero, we get an empty backing type instance.
-	for field, _ := range sectionExtensions {
+	for field := range sectionExtensions {
 		sections[field] = yaml.Node{}
 	}
 	err := yaml.Unmarshal(data, &sections)
diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go
index 99b617a8b..fc89cbfb9 100644
--- a/internals/plan/plan_test.go
+++ b/internals/plan/plan_test.go
@@ -2138,6 +2138,7 @@ func (s *S) TestSectionOrder(c *C) {
 		LogTargets: combined.LogTargets,
 	}
 	data, err := yaml.Marshal(plan)
+	c.Assert(err, IsNil)
 	c.Assert(string(data), Equals, string(reindent(`
 	services:
 		srv1:

From 628d838de8db259b0638f87f397e6c5e719fd04a Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 14 Oct 2024 21:47:10 +0200
Subject: [PATCH 07/15] staticcheck planstate

---
 internals/overlord/planstate/manager.go      | 2 +-
 internals/overlord/planstate/manager_test.go | 7 ++++---
 internals/overlord/planstate/package_test.go | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go
index a42650bd1..90f984d84 100644
--- a/internals/overlord/planstate/manager.go
+++ b/internals/overlord/planstate/manager.go
@@ -176,7 +176,7 @@ func (m *PlanManager) appendLayer(newLayer *plan.Layer, inner bool) (*plan.Plan,
 		// an order?
 		newSubLabel, _, hasSub := strings.Cut(newLayer.Label, "/")
 		lastIndex := layersCount - 1
-		for i, _ := range m.plan.Layers {
+		for i := range m.plan.Layers {
 			layer := m.plan.Layers[lastIndex-i]
 			layerSubLabel, _, _ := strings.Cut(layer.Label, "/")
 			// If we have a sub-directory match we know it already exists.
diff --git a/internals/overlord/planstate/manager_test.go b/internals/overlord/planstate/manager_test.go
index 25cf34cce..d2dd1f46a 100644
--- a/internals/overlord/planstate/manager_test.go
+++ b/internals/overlord/planstate/manager_test.go
@@ -383,7 +383,7 @@ test-field:
 	ps.planLayersHasLen(c, 3)
 
 	// Make sure that layer validation is happening.
-	layer, err = plan.ParseLayer(0, "label4", []byte(`
+	_, err = plan.ParseLayer(0, "label4", []byte(`
 checks:
     bad-check:
         override: replace
@@ -394,7 +394,7 @@ checks:
 	c.Check(err, ErrorMatches, `(?s).*plan check.*must be "alive" or "ready".*`)
 
 	// Make sure that layer validation is happening for extensions.
-	layer, err = plan.ParseLayer(0, "label4", []byte(`
+	_, err = plan.ParseLayer(0, "label4", []byte(`
 test-field:
     my1:
         override: replace
@@ -422,6 +422,7 @@ services:
         command: foo
 `)
 	err = ps.planMgr.AppendLayer(layer, false)
+	c.Assert(err, IsNil)
 
 	// Set arguments to services.
 	serviceArgs := map[string][]string{
@@ -455,8 +456,8 @@ func (ps *planSuite) TestChangeListenerAndLocking(c *C) {
 		// so we should be able to acquire it.
 		planLock := manager.PlanLock()
 		planLock.Lock()
+		calls++ // calls incremented here to satisfy staticcheck.
 		planLock.Unlock()
-		calls++
 	})
 
 	// Run operations in goroutine so we can time out the test if it fails.
diff --git a/internals/overlord/planstate/package_test.go b/internals/overlord/planstate/package_test.go
index 4e79d7e6f..a8c242728 100644
--- a/internals/overlord/planstate/package_test.go
+++ b/internals/overlord/planstate/package_test.go
@@ -147,7 +147,7 @@ type testSection struct {
 func (ts *testSection) Validate() error {
 	// Fictitious test requirement: fields must start with t
 	prefix := "t"
-	for field, _ := range ts.Entries {
+	for field := range ts.Entries {
 		if !strings.HasPrefix(field, prefix) {
 			return fmt.Errorf("%q entry names must start with %q", testField, prefix)
 		}

From 4f51210b220e76eca6f9a3b01acf5ac87d7ddc7d Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 14 Oct 2024 23:20:34 +0200
Subject: [PATCH 08/15] add plan tests

---
 internals/plan/plan.go      |  22 +++++
 internals/plan/plan_test.go | 161 ++++++++++++++++++++++++++++++++++++
 2 files changed, 183 insertions(+)

diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index c71698332..f80c57ed4 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -154,6 +154,28 @@ func (p *Plan) MarshalYAML() (interface{}, error) {
 	return plan, nil
 }
 
+// Layer represents an unmarshalled YAML layer configuration file. Layer files
+// are maintained as part of the Plan, ordered by their respective order
+// number. Each layer configuration also has a unique label, used for locating
+// and updating a specific layer configuration.
+//
+// Pebble supports a two level layer configuration directory structure. In the
+// root layers directory, both layer files and layer sub-directories are
+// allowed. Within a sub-directory, only layer files.
+//
+// ┌────────────────────────────┬─────────────────┬─────────┐
+// │ File (inside layersDir)    │ Order           │ Label   │
+// ├────────────────────────────┼─────────────────┼─────────┤
+// │                            │                 │         │
+// │ 001-foo.yaml               │ 001-000 => 1000 │ foo     │
+// │                            │                 │         │
+// │ 002-bar.d/001-aaa.yaml     │ 002-001 => 2001 │ bar/aaa │
+// │                            │                 │         │
+// │ 002-bar.d/002-bbb.yaml     │ 002-002 => 2002 │ bar/bbb │
+// │                            │                 │         │
+// │ 003-baz.yaml               │ 003-000 => 3000 │ baz     │
+// │                            │                 │         │
+// └────────────────────────────┴─────────────────┴─────────┘
 type Layer struct {
 	Order       int                   `yaml:"-"`
 	Label       string                `yaml:"-"`
diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go
index fc89cbfb9..fb03962ca 100644
--- a/internals/plan/plan_test.go
+++ b/internals/plan/plan_test.go
@@ -2157,3 +2157,164 @@ func (s *S) TestSectionOrder(c *C) {
 			services: []
 			override: replace`)))
 }
+
+// createLayerPath combines the base path with a configuration layer
+// filename (which may include a single sub-directory), and creates
+// the missing directories and an empty configuration file.
+func createLayerPath(c *C, base string, name string) {
+	path := filepath.Join(base, name)
+	err := os.MkdirAll(filepath.Dir(path), 0777)
+	c.Assert(err, IsNil)
+	file, err := os.OpenFile(path, os.O_RDONLY|os.O_CREATE, 0644)
+	c.Assert(err, IsNil)
+	err = file.Close()
+	c.Assert(err, IsNil)
+}
+
+var readDirTests = []struct {
+	summary    string
+	layerNames []string
+	orders     []int
+	labels     []string
+	error      string
+}{{
+	summary: "Invalid filename #1",
+	layerNames: []string{
+		"001foo.yaml",
+	},
+	error: ".*invalid layer filename.*",
+}, {
+	summary: "Invalid filename #2",
+	layerNames: []string{
+		"001-foo.d/001foo.yaml",
+	},
+	error: ".*invalid layer filename.*",
+}, {
+	summary: "Invalid sub-directory",
+	layerNames: []string{
+		"001foo.d/001-bar.yaml",
+	},
+	error: ".*invalid layer sub.*",
+}, {
+	summary: "Not unique order #1",
+	layerNames: []string{
+		"001-foo.yaml",
+		"001-bar.yaml",
+	},
+	error: ".*not unique.*",
+}, {
+	summary: "Not unique order #2",
+	layerNames: []string{
+		"002-dir.d/001-foo.yaml",
+		"002-dir.d/001-bar.yaml",
+	},
+	error: ".*not unique.*",
+}, {
+	summary: "Not unique label #1",
+	layerNames: []string{
+		"001-foo.yaml",
+		"002-foo.yaml",
+	},
+	error: ".*not unique.*",
+}, {
+	summary: "Not unique label #2",
+	layerNames: []string{
+		"002-dir.d/001-foo.yaml",
+		"002-dir.d/002-foo.yaml",
+	},
+	error: ".*not unique.*",
+}, {
+	summary: "Valid load",
+	layerNames: []string{
+		"001-aaa.yaml",
+		"010-bbb.yaml",
+		"002-dir.d/100-foo.yaml",
+		"002-dir.d/090-bar.yaml",
+		"008-ccc.yaml",
+		"900-overlay.d/002-something.yaml",
+		"900-overlay.d/001-else.yaml",
+		"003-plans.d/999-final.yaml",
+		"009-baz.d/009-baz.yaml",
+	},
+	orders: []int{
+		1000,
+		2090,
+		2100,
+		3999,
+		8000,
+		9009,
+		10000,
+		900001,
+		900002,
+	},
+	labels: []string{
+		"aaa",
+		"dir/bar",
+		"dir/foo",
+		"plans/final",
+		"ccc",
+		"baz/baz",
+		"bbb",
+		"overlay/else",
+		"overlay/something",
+	},
+}}
+
+func (s *S) TestReadLayersDir(c *C) {
+	for _, test := range readDirTests {
+		c.Logf("Running ReadLayersDir: %s", test.summary)
+
+		tempDir := c.MkDir()
+		for _, layerName := range test.layerNames {
+			createLayerPath(c, tempDir, layerName)
+		}
+
+		layers, err := plan.ReadLayersDir(tempDir)
+		if test.error != "" || err != nil {
+			c.Assert(err, ErrorMatches, test.error)
+		}
+
+		c.Assert(len(layers), Equals, len(test.labels))
+		c.Assert(len(layers), Equals, len(test.orders))
+
+		for i, layer := range layers {
+			c.Assert(layer.Order, Equals, test.orders[i])
+			c.Assert(layer.Label, Equals, test.labels[i])
+		}
+	}
+}
+
+func (s *S) TestParseLayerLabelValidatorOK(c *C) {
+	for _, label := range []string{
+		"f12345",
+		"foo",
+		"f-1-2-3-4",
+		"foo-bar-baz-12345",
+		"f12345/f12345",
+		"foo/foo",
+		"f-1-2-3-4/f-1-2-3-4",
+		"foo-bar-baz-12345/foo-bar-baz-12345",
+	} {
+		_, err := plan.ParseLayer(0, label, []byte(""))
+		c.Assert(err, IsNil)
+	}
+}
+
+func (s *S) TestParseLayerLabelValidatorFail(c *C) {
+	for _, label := range []string{
+		"0foo",
+		"01234",
+		"f",
+		"foo--bar",
+		"fooBar",
+		"foo/0foo",
+		"foo/01234",
+		"foo/f",
+		"foo/foo--bar",
+		"foo/fooBar",
+		"foo/bar/baz",
+	} {
+		_, err := plan.ParseLayer(0, label, []byte(""))
+		c.Assert(err, ErrorMatches, ".*invalid label.*")
+	}
+}

From 11e246eb4078956382295b35fc87f03af9fbfd8e Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 14 Oct 2024 23:33:00 +0200
Subject: [PATCH 09/15] add planstate explanations

---
 internals/overlord/planstate/manager.go | 26 +++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go
index 90f984d84..7582146dc 100644
--- a/internals/overlord/planstate/manager.go
+++ b/internals/overlord/planstate/manager.go
@@ -162,6 +162,32 @@ func (m *PlanManager) CombineLayer(layer *plan.Layer, inner bool) error {
 	return nil
 }
 
+// appendLayer appends (or inserts) a new layer configuration
+// into the layers slice of the current plan. One important
+// task of this method is to determine the new order of the layer.
+//
+// ┌────────────────────────────┬─────────────────┬─────────┐
+// │ Example Layers slice       │ Order           │ Label   │
+// ├────────────────────────────┼─────────────────┼─────────┤
+// │                            │                 │         │
+// │ 001-foo.yaml               │ 001-000 => 1000 │ foo     │
+// │                            │                 │         │
+// │ 002-bar.d/001-aaa.yaml     │ 002-001 => 2001 │ bar/aaa │
+// │                            │                 │         │
+// │ 002-bar.d/002-bbb.yaml     │ 002-002 => 2002 │ bar/bbb │
+// │                            │                 │         │
+// │ 003-baz.yaml               │ 003-000 => 3000 │ baz     │
+// │                            │                 │         │
+// └────────────────────────────┴─────────────────┴─────────┘
+//
+// The new incoming layer only supplies the label, which may include
+// a sub-directory prefix. Normally without a sub-directory prefix,
+// the new layer will always be appended, which means incrementing
+// the root level order (+ 1000). If a sub-directory already exist
+// in the layers slice, its order was already allocated, which means
+// we can at most insert the layer as the last entry in the
+// sub-directory. However, this insert is only allowed if explicitly
+// requested by the user (inner=true).
 func (m *PlanManager) appendLayer(newLayer *plan.Layer, inner bool) (*plan.Plan, error) {
 	// The starting index and order assumes no existing layers.
 	newIndex := 0

From f683d93a03e0abc0d945e62adcad48b30e38021a Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Tue, 15 Oct 2024 00:05:27 +0200
Subject: [PATCH 10/15] planstate unit tests

---
 internals/overlord/planstate/manager.go      |  1 +
 internals/overlord/planstate/manager_test.go | 79 ++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go
index 7582146dc..d23ad923d 100644
--- a/internals/overlord/planstate/manager.go
+++ b/internals/overlord/planstate/manager.go
@@ -211,6 +211,7 @@ func (m *PlanManager) appendLayer(newLayer *plan.Layer, inner bool) (*plan.Plan,
 			if layerSubLabel == newSubLabel {
 				newOrder = layer.Order + 1
 				newIndex = lastIndex - i + 1
+				break
 			}
 		}
 
diff --git a/internals/overlord/planstate/manager_test.go b/internals/overlord/planstate/manager_test.go
index d2dd1f46a..ba07164c6 100644
--- a/internals/overlord/planstate/manager_test.go
+++ b/internals/overlord/planstate/manager_test.go
@@ -509,3 +509,82 @@ services:
 
 	c.Assert(calls, Equals, 5)
 }
+
+func (ps *planSuite) TestAppendLayersNoInner(c *C) {
+	plan.RegisterSectionExtension(testField, testExtension{})
+	defer plan.UnregisterSectionExtension(testField)
+	var err error
+	ps.planMgr, err = planstate.NewManager(ps.layersDir)
+	c.Assert(err, IsNil)
+
+	layer := ps.parseLayer(c, 0, "foo/bar", "")
+	err = ps.planMgr.AppendLayer(layer, false)
+	c.Assert(err, IsNil)
+	layer = ps.parseLayer(c, 0, "baz", "")
+	err = ps.planMgr.AppendLayer(layer, false)
+	c.Assert(err, IsNil)
+	layer = ps.parseLayer(c, 0, "foo/baz", "")
+	err = ps.planMgr.AppendLayer(layer, false)
+	c.Assert(err, ErrorMatches, ".*cannot insert sub-directory.*")
+}
+
+func (ps *planSuite) TestAppendLayersOrderAllocation(c *C) {
+	plan.RegisterSectionExtension(testField, testExtension{})
+	defer plan.UnregisterSectionExtension(testField)
+	var err error
+	ps.planMgr, err = planstate.NewManager(ps.layersDir)
+	c.Assert(err, IsNil)
+
+	appendLabels := []string{
+		"foo",
+		"baz/aaa",
+		"something",
+		"baz/bbb",
+		"else",
+		"baz/ccc",
+		"zzz/zzz",
+		"final",
+	}
+
+	for _, label := range appendLabels {
+		layer := ps.parseLayer(c, 0, label, "")
+		err = ps.planMgr.AppendLayer(layer, true)
+		c.Assert(err, IsNil)
+	}
+
+	layersResult := []struct {
+		label string
+		order int
+	}{{
+		label: "foo",
+		order: 1000,
+	}, {
+		label: "baz/aaa",
+		order: 2001,
+	}, {
+		label: "baz/bbb",
+		order: 2002,
+	}, {
+		label: "baz/ccc",
+		order: 2003,
+	}, {
+		label: "something",
+		order: 3000,
+	}, {
+		label: "else",
+		order: 4000,
+	}, {
+		label: "zzz/zzz",
+		order: 5001,
+	}, {
+		label: "final",
+		order: 6000,
+	}}
+
+	plan := ps.planMgr.Plan()
+	// Check the layers order and each layer's order is correct.
+	for i, layer := range layersResult {
+		c.Assert(plan.Layers[i].Order, Equals, layer.order)
+		c.Assert(plan.Layers[i].Label, Equals, layer.label)
+	}
+}

From 9652ba32764ae6ac2f055f246d298e82141cf745 Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Tue, 15 Oct 2024 00:15:24 +0200
Subject: [PATCH 11/15] tweak

---
 internals/overlord/planstate/manager.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go
index d23ad923d..c4f0951f1 100644
--- a/internals/overlord/planstate/manager.go
+++ b/internals/overlord/planstate/manager.go
@@ -222,7 +222,7 @@ func (m *PlanManager) appendLayer(newLayer *plan.Layer, inner bool) (*plan.Plan,
 			newIndex = layersCount
 			newOrder = ((m.plan.Layers[layersCount-1].Order / 1000) + 1) * 1000
 			if hasSub {
-				// The file in the sub-directory needs an order to "001".
+				// The first file in the sub-directory gets an order of "001".
 				newOrder += 1
 			}
 		}

From 4f4cb6d025ccac48534c61b35a150cfc1c7e9b3a Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 21 Oct 2024 08:40:17 +0200
Subject: [PATCH 12/15] Review feedback group 2

---
 internals/plan/plan.go      | 87 ++++++++++++-------------------------
 internals/plan/plan_test.go | 27 +++++++++++-
 2 files changed, 54 insertions(+), 60 deletions(-)

diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index f80c57ed4..1913acd8d 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -159,23 +159,11 @@ func (p *Plan) MarshalYAML() (interface{}, error) {
 // number. Each layer configuration also has a unique label, used for locating
 // and updating a specific layer configuration.
 //
-// Pebble supports a two level layer configuration directory structure. In the
+// Pebble supports a two-level layer configuration directory structure. In the
 // root layers directory, both layer files and layer sub-directories are
-// allowed. Within a sub-directory, only layer files.
+// allowed. Within a sub-directory, only layer files are allowed.
 //
-// ┌────────────────────────────┬─────────────────┬─────────┐
-// │ File (inside layersDir)    │ Order           │ Label   │
-// ├────────────────────────────┼─────────────────┼─────────┤
-// │                            │                 │         │
-// │ 001-foo.yaml               │ 001-000 => 1000 │ foo     │
-// │                            │                 │         │
-// │ 002-bar.d/001-aaa.yaml     │ 002-001 => 2001 │ bar/aaa │
-// │                            │                 │         │
-// │ 002-bar.d/002-bbb.yaml     │ 002-002 => 2002 │ bar/bbb │
-// │                            │                 │         │
-// │ 003-baz.yaml               │ 003-000 => 3000 │ baz     │
-// │                            │                 │         │
-// └────────────────────────────┴─────────────────┴─────────┘
+// Please see ReadLayersDir for more details.
 type Layer struct {
 	Order       int                   `yaml:"-"`
 	Label       string                `yaml:"-"`
@@ -1249,25 +1237,20 @@ func (p *Plan) checkCycles() error {
 	return err
 }
 
-// labelExp represents a match of a valid layer label, which may include a
+// labelRegexp represents a match of a valid layer label, which may include a
 // directory prefix (which excludes the '.d' ending, in the same way the
 // file suffix is omitted).
 //
-// ┌─────────┬─────────────────────────────────────┐
-// │ Label   │ Description                         │
-// ├─────────┼─────────────────────────────────────┤
-// │         │                                     │
-// │ abc     │ Label of file in layers root        │
-// │         │                                     │
-// │ foo/bar │ Label of file inside sub-directory  │
-// │         │                                     │
-// └─────────┴─────────────────────────────────────┘
-var labelExp = regexp.MustCompile(`^(([a-z](?:-?[a-z0-9]){2,})/)?([a-z](?:-?[a-z0-9]){2,})$`)
+//	| Label    | Description                        |
+//	| -------- | ---------------------------------- |
+//	| abc      | Label of file in layers root       |
+//	| foo/bar  | Label of file inside sub-directory |
+var labelRegexp = regexp.MustCompile(`^(([a-z](?:-?[a-z0-9]){2,})/)?([a-z](?:-?[a-z0-9]){2,})$`)
 
 func ParseLayer(order int, label string, data []byte) (*Layer, error) {
 	// This function can be called directly over the daemon API. We
 	// must fail the API request if the label is not valid.
-	match := labelExp.FindStringSubmatch(label)
+	match := labelRegexp.FindStringSubmatch(label)
 	if match == nil {
 		return nil, fmt.Errorf("cannot parse layer: invalid label %q", label)
 	}
@@ -1395,18 +1378,15 @@ func validServiceAction(action ServiceAction, additionalValid ...ServiceAction)
 
 // ReadLayersDir loads the YAML layer files from the first two directory
 // levels starting at layersDir in the order as specified by the order
-// directory and file prefixes. Note that the directory and file suffix
-// is dropped for the label.
+// directory and file order prefixes. The directory and file suffixes
+// are dropped in the returned labels.
 //
-// ┌────────────────────────┬──────────────────┬─────────┐
-// │ Layer Path             │ Order            │ Label   │
-// ├────────────────────────┼──────────────────┼─────────┤
-// │                        │                  │         │
-// │ 001-abc.yaml           │ 001-000 => 1000  │ abc     │
-// │                        │                  │         │
-// │ 002-foo.d/001-bar.yaml │ 002-001 => 2001  │ foo/bar │
-// │                        │                  │         │
-// └────────────────────────┴──────────────────┴─────────┘
+//	| File (inside layersDir)    | Order           | Label   |
+//	| -------------------------- | --------------- | ------- |
+//	| 001-foo.yaml               | 001-000 => 1000 | foo     |
+//	| 002-bar.d/001-aaa.yaml     | 002-001 => 2001 | bar/aaa |
+//	| 002-bar.d/002-bbb.yaml     | 002-002 => 2002 | bar/bbb |
+//	| 003-baz.yaml               | 003-000 => 3000 | baz     |
 func ReadLayersDir(layersDir string) ([]*Layer, error) {
 	var layers []*Layer
 
@@ -1434,7 +1414,7 @@ func ReadLayersDir(layersDir string) ([]*Layer, error) {
 
 			// Add the config files from the second level
 			for _, l2Entry := range l2Entries {
-				layer, err := configLayerLoad(layersDir, l1Entry, l2Entry)
+				layer, err := loadConfigLayer(layersDir, l1Entry, l2Entry)
 				if err != nil {
 					return nil, err
 				}
@@ -1442,7 +1422,7 @@ func ReadLayersDir(layersDir string) ([]*Layer, error) {
 			}
 		} else {
 			// Add the config files from the first level
-			layer, err := configLayerLoad(layersDir, l1Entry, nil)
+			layer, err := loadConfigLayer(layersDir, l1Entry, nil)
 			if err != nil {
 				return nil, err
 			}
@@ -1453,12 +1433,12 @@ func ReadLayersDir(layersDir string) ([]*Layer, error) {
 	return layers, nil
 }
 
-// configLayerLoad loads a layer configuration file and returns a Layer
+// loadConfigLayer loads a layer configuration file and returns a Layer
 // on success. The layer configuration is typically in the Pebble layers
 // root directory, in which case l2Entry must be nil. If the file is
 // inside a sub-directory, l1Entry must supply information on the directory,
 // while l2Entry information on the file name itself.
-func configLayerLoad(layersDir string, l1Entry *configEntry, l2Entry *configEntry) (*Layer, error) {
+func loadConfigLayer(layersDir string, l1Entry *configEntry, l2Entry *configEntry) (*Layer, error) {
 	// Resolve the order and label, which may include additional
 	// information from an optional sub-directory prefix.
 	label := l1Entry.label
@@ -1466,7 +1446,7 @@ func configLayerLoad(layersDir string, l1Entry *configEntry, l2Entry *configEntr
 	path := filepath.Join(layersDir, l1Entry.name)
 	if l2Entry != nil {
 		// Config layer is inside a sub-directory.
-		label = filepath.Join(label, l2Entry.label)
+		label = label + "/" + l2Entry.label
 		order = order + l2Entry.order
 		path = filepath.Join(path, l2Entry.name)
 	}
@@ -1485,21 +1465,10 @@ func configLayerLoad(layersDir string, l1Entry *configEntry, l2Entry *configEntr
 	return layer, nil
 }
 
-// configEntryExp matches either a valid config layer YAML file name or it
-// matches a valid config layer directory.
-//
-// ┌────────────────────────────────────────────────────────┐
-// │ Match Index                                            │
-// ├────────────────────────┬───────────┬───────────┬───────┤
-// │ 0 (Supplied Name)      │ 1 (order) │ 2 (label) │ 3     │
-// ├────────────────────────┼───────────┼───────────┼───────┤
-// │                        │           │           │       │
-// │ 001-abc.yaml           │ 001       │ abc       │ .yaml │
-// │                        │           │           │       │
-// │ 002-foo.d              │ 002       │ foo       │ .d    │
-// │                        │           │           │       │
-// └────────────────────────┴───────────┴───────────┴───────┘
-var configEntryExp = regexp.MustCompile(`^([0-9]{3})-([a-z](?:-?[a-z0-9]){2,})(.yaml|.d)$`)
+// configEntryRegexp matches either a valid config layer YAML file name or a
+// valid config layer directory. Match[1] is the 3-digit order and match[2]
+// is the label.
+var configEntryRegexp = regexp.MustCompile(`^([0-9]{3})-([a-z](?:-?[a-z0-9]){2,})(.yaml|.d)$`)
 
 type configEntry struct {
 	name  string
@@ -1534,7 +1503,7 @@ func configLayerEntries(configDir string, dirOK bool) (configs []*configEntry, e
 		}
 
 		// Let's ensure the file or sub-directory name is valid.
-		match := configEntryExp.FindStringSubmatch(entry.Name())
+		match := configEntryRegexp.FindStringSubmatch(entry.Name())
 		if match == nil {
 			if info.IsDir() {
 				return nil, fmt.Errorf("invalid layer sub-directory name: %q (must look like \"123-some-label.d\")", entry.Name())
diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go
index fb03962ca..36b5b41e7 100644
--- a/internals/plan/plan_test.go
+++ b/internals/plan/plan_test.go
@@ -20,6 +20,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"slices"
 	"strings"
 	"time"
 	"unicode"
@@ -2165,7 +2166,15 @@ func createLayerPath(c *C, base string, name string) {
 	path := filepath.Join(base, name)
 	err := os.MkdirAll(filepath.Dir(path), 0777)
 	c.Assert(err, IsNil)
-	file, err := os.OpenFile(path, os.O_RDONLY|os.O_CREATE, 0644)
+	file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0644)
+	c.Assert(err, IsNil)
+	// Let's mix in the layer name into the command so we can
+	// verify the correct layer has the correct file content.
+	_, err = file.Write(reindent(fmt.Sprintf(`
+	services:
+		srv:
+			override: replace
+			command: %s`, name)))
 	c.Assert(err, IsNil)
 	err = file.Close()
 	c.Assert(err, IsNil)
@@ -2265,6 +2274,7 @@ func (s *S) TestReadLayersDir(c *C) {
 		c.Logf("Running ReadLayersDir: %s", test.summary)
 
 		tempDir := c.MkDir()
+
 		for _, layerName := range test.layerNames {
 			createLayerPath(c, tempDir, layerName)
 		}
@@ -2280,6 +2290,21 @@ func (s *S) TestReadLayersDir(c *C) {
 		for i, layer := range layers {
 			c.Assert(layer.Order, Equals, test.orders[i])
 			c.Assert(layer.Label, Equals, test.labels[i])
+
+			// Let's make sure each file contains the expected
+			// command. This will confirm the content is loaded
+			// from the correct file.
+			ordered := make([]string, 0, len(test.layerNames))
+			ordered = append(ordered, test.layerNames...)
+			slices.Sort(ordered)
+
+			c.Assert(layer.Services, DeepEquals, map[string]*plan.Service{
+				"srv": {
+					Name:     "srv",
+					Override: "replace",
+					Command:  ordered[i],
+				},
+			})
 		}
 	}
 }

From 7d27f2a27ea2e4040ab56b8b9798e1f896b23d75 Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 21 Oct 2024 10:52:44 +0200
Subject: [PATCH 13/15] Review feedback group 3

---
 internals/overlord/planstate/manager.go      | 28 +++++++-------------
 internals/overlord/planstate/manager_test.go |  4 +--
 2 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go
index c4f0951f1..568325628 100644
--- a/internals/overlord/planstate/manager.go
+++ b/internals/overlord/planstate/manager.go
@@ -166,24 +166,17 @@ func (m *PlanManager) CombineLayer(layer *plan.Layer, inner bool) error {
 // into the layers slice of the current plan. One important
 // task of this method is to determine the new order of the layer.
 //
-// ┌────────────────────────────┬─────────────────┬─────────┐
-// │ Example Layers slice       │ Order           │ Label   │
-// ├────────────────────────────┼─────────────────┼─────────┤
-// │                            │                 │         │
-// │ 001-foo.yaml               │ 001-000 => 1000 │ foo     │
-// │                            │                 │         │
-// │ 002-bar.d/001-aaa.yaml     │ 002-001 => 2001 │ bar/aaa │
-// │                            │                 │         │
-// │ 002-bar.d/002-bbb.yaml     │ 002-002 => 2002 │ bar/bbb │
-// │                            │                 │         │
-// │ 003-baz.yaml               │ 003-000 => 3000 │ baz     │
-// │                            │                 │         │
-// └────────────────────────────┴─────────────────┴─────────┘
+//	| File (inside layersDir)    | Order           | Label   |
+//	| -------------------------- | --------------- | ------- |
+//	| 001-foo.yaml               | 001-000 => 1000 | foo     |
+//	| 002-bar.d/001-aaa.yaml     | 002-001 => 2001 | bar/aaa |
+//	| 002-bar.d/002-bbb.yaml     | 002-002 => 2002 | bar/bbb |
+//	| 003-baz.yaml               | 003-000 => 3000 | baz     |
 //
 // The new incoming layer only supplies the label, which may include
 // a sub-directory prefix. Normally without a sub-directory prefix,
 // the new layer will always be appended, which means incrementing
-// the root level order (+ 1000). If a sub-directory already exist
+// the root level order (+ 1000). If a sub-directory already exists
 // in the layers slice, its order was already allocated, which means
 // we can at most insert the layer as the last entry in the
 // sub-directory. However, this insert is only allowed if explicitly
@@ -201,16 +194,15 @@ func (m *PlanManager) appendLayer(newLayer *plan.Layer, inner bool) (*plan.Plan,
 		// sub-directory that exists and for which we already allocated
 		// an order?
 		newSubLabel, _, hasSub := strings.Cut(newLayer.Label, "/")
-		lastIndex := layersCount - 1
-		for i := range m.plan.Layers {
-			layer := m.plan.Layers[lastIndex-i]
+		for i := layersCount - 1; i >= 0; i-- {
+			layer := m.plan.Layers[i]
 			layerSubLabel, _, _ := strings.Cut(layer.Label, "/")
 			// If we have a sub-directory match we know it already exists.
 			// Since we searched backwards, we know the order should be the
 			// next integer value.
 			if layerSubLabel == newSubLabel {
 				newOrder = layer.Order + 1
-				newIndex = lastIndex - i + 1
+				newIndex = i + 1
 				break
 			}
 		}
diff --git a/internals/overlord/planstate/manager_test.go b/internals/overlord/planstate/manager_test.go
index ba07164c6..54d1879f4 100644
--- a/internals/overlord/planstate/manager_test.go
+++ b/internals/overlord/planstate/manager_test.go
@@ -510,7 +510,7 @@ services:
 	c.Assert(calls, Equals, 5)
 }
 
-func (ps *planSuite) TestAppendLayersNoInner(c *C) {
+func (ps *planSuite) TestAppendLayersWithoutInner(c *C) {
 	plan.RegisterSectionExtension(testField, testExtension{})
 	defer plan.UnregisterSectionExtension(testField)
 	var err error
@@ -528,7 +528,7 @@ func (ps *planSuite) TestAppendLayersNoInner(c *C) {
 	c.Assert(err, ErrorMatches, ".*cannot insert sub-directory.*")
 }
 
-func (ps *planSuite) TestAppendLayersOrderAllocation(c *C) {
+func (ps *planSuite) TestAppendLayersWithInner(c *C) {
 	plan.RegisterSectionExtension(testField, testExtension{})
 	defer plan.UnregisterSectionExtension(testField)
 	var err error

From c33576c20ca80d9ba443400a2f559b70a30b95e6 Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 21 Oct 2024 11:14:48 +0200
Subject: [PATCH 14/15] Review feedback group 4

---
 client/plan.go      |  2 +-
 client/plan_test.go | 24 ++++++++++++++++++++----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/client/plan.go b/client/plan.go
index 8add24f0b..b6cc6a259 100644
--- a/client/plan.go
+++ b/client/plan.go
@@ -25,7 +25,7 @@ type AddLayerOptions struct {
 	// has the given label. False (the default) means append a new layer.
 	Combine bool
 
-	// Inner true means a new layer append may go into an existing
+	// Inner set to true means a new layer append may go into an existing
 	// subdirectory, even though it may not result in appending it
 	// to the end of the layers slice (it becomes an insert).
 	Inner bool
diff --git a/client/plan_test.go b/client/plan_test.go
index 8e55a5f32..38fd081a4 100644
--- a/client/plan_test.go
+++ b/client/plan_test.go
@@ -24,7 +24,22 @@ import (
 )
 
 func (cs *clientSuite) TestAddLayer(c *check.C) {
-	for _, combine := range []bool{false, true} {
+	for _, option := range []struct {
+		combine bool
+		inner   bool
+	}{{
+		combine: false,
+		inner:   false,
+	}, {
+		combine: true,
+		inner:   false,
+	}, {
+		combine: false,
+		inner:   true,
+	}, {
+		combine: true,
+		inner:   true,
+	}} {
 		cs.rsp = `{
 		"type": "sync",
 		"status-code": 200,
@@ -37,7 +52,8 @@ services:
         command: cmd
 `[1:]
 		err := cs.cli.AddLayer(&client.AddLayerOptions{
-			Combine:   combine,
+			Combine:   option.combine,
+			Inner:     option.inner,
 			Label:     "foo",
 			LayerData: []byte(layerYAML),
 		})
@@ -49,11 +65,11 @@ services:
 		c.Assert(json.NewDecoder(cs.req.Body).Decode(&body), check.IsNil)
 		c.Assert(body, check.DeepEquals, map[string]interface{}{
 			"action":  "add",
-			"combine": combine,
+			"combine": option.combine,
 			"label":   "foo",
 			"format":  "yaml",
 			"layer":   layerYAML,
-			"inner":   false,
+			"inner":   option.inner,
 		})
 	}
 }

From 927116ff8454de85ed7eee2a335bef8bd1aa3182 Mon Sep 17 00:00:00 2001
From: Fred Lotter <fred.lotter@canonical.com>
Date: Mon, 21 Oct 2024 11:54:51 +0200
Subject: [PATCH 15/15] remove API layer validation for now

---
 client/plan.go              |  8 ++++++++
 internals/plan/plan.go      | 17 -----------------
 internals/plan/plan_test.go | 35 -----------------------------------
 3 files changed, 8 insertions(+), 52 deletions(-)

diff --git a/client/plan.go b/client/plan.go
index b6cc6a259..9c34f58e7 100644
--- a/client/plan.go
+++ b/client/plan.go
@@ -55,6 +55,14 @@ func (client *Client) AddLayer(opts *AddLayerOptions) error {
 		Format:  "yaml",
 		Layer:   string(opts.LayerData),
 	}
+
+	// Add label validation here once layer persistence is supported over
+	// the API. We cannot do this in the plan library because JUJU already
+	// has labels in production systems that violates the layers file
+	// naming convention (which includes the label). Since JUJU uses its
+	// own client, we can enforce the label naming convention on all other
+	// systems using the Pebble supplied client by validating it here.
+
 	var body bytes.Buffer
 	if err := json.NewEncoder(&body).Encode(&payload); err != nil {
 		return err
diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index 1913acd8d..3d65675fe 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -1237,24 +1237,7 @@ func (p *Plan) checkCycles() error {
 	return err
 }
 
-// labelRegexp represents a match of a valid layer label, which may include a
-// directory prefix (which excludes the '.d' ending, in the same way the
-// file suffix is omitted).
-//
-//	| Label    | Description                        |
-//	| -------- | ---------------------------------- |
-//	| abc      | Label of file in layers root       |
-//	| foo/bar  | Label of file inside sub-directory |
-var labelRegexp = regexp.MustCompile(`^(([a-z](?:-?[a-z0-9]){2,})/)?([a-z](?:-?[a-z0-9]){2,})$`)
-
 func ParseLayer(order int, label string, data []byte) (*Layer, error) {
-	// This function can be called directly over the daemon API. We
-	// must fail the API request if the label is not valid.
-	match := labelRegexp.FindStringSubmatch(label)
-	if match == nil {
-		return nil, fmt.Errorf("cannot parse layer: invalid label %q", label)
-	}
-
 	layer := &Layer{
 		Services:   make(map[string]*Service),
 		Checks:     make(map[string]*Check),
diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go
index 36b5b41e7..000e0c30e 100644
--- a/internals/plan/plan_test.go
+++ b/internals/plan/plan_test.go
@@ -2308,38 +2308,3 @@ func (s *S) TestReadLayersDir(c *C) {
 		}
 	}
 }
-
-func (s *S) TestParseLayerLabelValidatorOK(c *C) {
-	for _, label := range []string{
-		"f12345",
-		"foo",
-		"f-1-2-3-4",
-		"foo-bar-baz-12345",
-		"f12345/f12345",
-		"foo/foo",
-		"f-1-2-3-4/f-1-2-3-4",
-		"foo-bar-baz-12345/foo-bar-baz-12345",
-	} {
-		_, err := plan.ParseLayer(0, label, []byte(""))
-		c.Assert(err, IsNil)
-	}
-}
-
-func (s *S) TestParseLayerLabelValidatorFail(c *C) {
-	for _, label := range []string{
-		"0foo",
-		"01234",
-		"f",
-		"foo--bar",
-		"fooBar",
-		"foo/0foo",
-		"foo/01234",
-		"foo/f",
-		"foo/foo--bar",
-		"foo/fooBar",
-		"foo/bar/baz",
-	} {
-		_, err := plan.ParseLayer(0, label, []byte(""))
-		c.Assert(err, ErrorMatches, ".*invalid label.*")
-	}
-}