Skip to content

Commit d617e09

Browse files
authored
Make subpackage name unique in independent subpackage Kptfiles (#1056)
* Rebased due to move to kptdev Signed-off-by: liamfallon <liam.fallon@est.tech> * Revert subpckage-dir parameter on upgrade Signed-off-by: liamfallon <liam.fallon@est.tech> * Addrees review comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Correct annotations for code generation Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Addressed copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Rebased due to move to kptdev Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Add small change to trigger CI on kptdev/porch Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Implement subpackage supoort Signed-off-by: liamfallon <liam.fallon@est.tech> * Remove files leaked in from implementation Signed-off-by: liamfallon <liam.fallon@est.tech> * added subpackage server side implmentation Signed-off-by: liamfallon <liam.fallon@est.tech> * Fix copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Fix copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Fix copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Fix copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Merged changes from API PR Signed-off-by: liamfallon <liam.fallon@est.tech> * Rebase to main Signed-off-by: liamfallon <liam.fallon@est.tech> * Update implementation Signed-off-by: liamfallon <liam.fallon@est.tech> * v1 generated yaml Signed-off-by: liamfallon <liam.fallon@est.tech> * Unit tests added Signed-off-by: liamfallon <liam.fallon@est.tech> * FIrst pushed version of code Signed-off-by: liamfallon <liam.fallon@est.tech> * Add tests for upgrade Signed-off-by: liamfallon <liam.fallon@est.tech> * Updates due to e2e test checks Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address lint errors Signed-off-by: liamfallon <liam.fallon@est.tech> * Upgraded to deal with CoPilot and lint errors Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comment Signed-off-by: liamfallon <liam.fallon@est.tech> * Fix unit test to address copilot comment Signed-off-by: liamfallon <liam.fallon@est.tech> * First cut of porchctl for subpkg Signed-off-by: liamfallon <liam.fallon@est.tech> * Implemented upgrade on porchctl Signed-off-by: liamfallon <liam.fallon@est.tech> * Updated for SoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Rebase against merged changes Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Added unit tests Signed-off-by: liamfallon <liam.fallon@est.tech> * Updated for CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Fix e2e tests Signed-off-by: liamfallon <liam.fallon@est.tech> * Get current e2e cli tests working Signed-off-by: liamfallon <liam.fallon@est.tech> * Rebase against main Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Add first draft of e2e cli tests for subpackage support Signed-off-by: liamfallon <liam.fallon@est.tech> * e2e tests upgrade Signed-off-by: liamfallon <liam.fallon@est.tech> * Updated e2e cli tests Signed-off-by: liamfallon <liam.fallon@est.tech> * Added checks for references in kptfiles of subpackages Signed-off-by: liamfallon <liam.fallon@est.tech> * Addressed CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Addressed copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Documentation for subpackage clone and upgrade in Porch Signed-off-by: liamfallon <liam.fallon@est.tech> * Updated docs generated by AI Signed-off-by: liamfallon <liam.fallon@est.tech> * Update documentation following proofreading Signed-off-by: liamfallon <liam.fallon@est.tech> * Fix typos found by CoPilot Signed-off-by: liamfallon <liam.fallon@est.tech> * Address CoPilot COmments Signed-off-by: liamfallon <liam.fallon@est.tech> * Rebase against merged porchctl implementation Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Use subdomain handling for subpackage names Signed-off-by: liamfallon <liam.fallon@est.tech> * Rebased against main Signed-off-by: liamfallon <liam.fallon@est.tech> * Resore change to command.go Signed-off-by: liamfallon <liam.fallon@est.tech> * Adddress copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Update e2e tests for domain based subpackage names Signed-off-by: liamfallon <liam.fallon@est.tech> * Deleted check that can never be reached Signed-off-by: liamfallon <liam.fallon@est.tech> * Resolve CoPilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> * Address copilot comments Signed-off-by: liamfallon <liam.fallon@est.tech> --------- Signed-off-by: liamfallon <liam.fallon@est.tech>
1 parent 59b2db6 commit d617e09

13 files changed

Lines changed: 288 additions & 104 deletions

File tree

api/porch/v1alpha1/util.go

Lines changed: 46 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,15 @@ import (
1919
"regexp"
2020
"slices"
2121
"strings"
22+
23+
pkgerrors "github.com/pkg/errors"
24+
"k8s.io/apimachinery/pkg/util/validation"
2225
)
2326

24-
// Valid relative paths should not start with '/', should not contain '..' components,
25-
// and should only contain valid path characters
27+
// validRelativePathRegex validates the basic shape of a relative path (slash-separated segments made of allowed characters).
28+
// Additional constraints (e.g. no leading/trailing '/', no '.', and DNS1123-compliant name composition) are enforced in IsValidSubpackageDir.
2629
var validRelativePathRegex = regexp.MustCompile(`^(?:[a-zA-Z0-9._-]+(?:/[a-zA-Z0-9._-]+)*)?$`)
2730

28-
// Check that there are no '..' components in a path
29-
var noDoubleDots = regexp.MustCompile(`(^|/)\.\.(/|$)`)
30-
3131
func (pr *PackageRevision) IsPublished() bool {
3232
return LifecycleIsPublished(pr.Spec.Lifecycle)
3333
}
@@ -105,10 +105,49 @@ func GetSubpackageDir(pkgRev *PackageRevision) (string, error) {
105105
}
106106

107107
subpackageDir := getSubpackageDir(pkgRev.Spec.Tasks[1])
108-
if IsValidSubpackageDir(subpackageDir) {
108+
if err := IsValidSubpackageDir(subpackageDir); err == nil {
109109
return subpackageDir, nil
110110
} else {
111-
return "", fmt.Errorf("subpackage directory %q is invalid", subpackageDir)
111+
return "", err
112+
}
113+
}
114+
115+
// IsValidSubpackageDir returns an error if subpackageDir is invalid.
116+
func IsValidSubpackageDir(subpackageDir string) error {
117+
// Empty string is invalid, a subpackage directory must be a relative path.
118+
if subpackageDir == "" {
119+
return pkgerrors.Errorf("subpackage directory %q is invalid", subpackageDir)
120+
}
121+
122+
// Check basic format and ensure it doesn't start with '/', doesn't end with '/', and doesn't contain '.'
123+
if subpackageDir[0] == '/' || strings.HasSuffix(subpackageDir, "/") || strings.Contains(subpackageDir, ".") {
124+
return pkgerrors.Errorf("subpackage directory %q is invalid, it cannot contain '.' or start with '/' or end with '/'", subpackageDir)
125+
}
126+
127+
if !validRelativePathRegex.MatchString(subpackageDir) {
128+
return pkgerrors.Errorf("subpackage directory %q is invalid, it must match regular expression %q", subpackageDir, validRelativePathRegex.String())
129+
}
130+
131+
if _, err := ComposeSubpkgObjName(subpackageDir); err != nil {
132+
return err
133+
}
134+
135+
return nil
136+
}
137+
138+
func ComposeSubpkgObjName(subpackageDir string) (string, error) {
139+
if subpackageDir == "" {
140+
return "", pkgerrors.Errorf("subpackage directory %q is invalid", subpackageDir)
141+
}
142+
143+
subpackageName := strings.ReplaceAll(subpackageDir, "/", ".")
144+
145+
objNameErrs := validation.IsDNS1123Subdomain(subpackageName)
146+
147+
if len(objNameErrs) == 0 {
148+
return subpackageName, nil
149+
} else {
150+
return "", pkgerrors.Errorf("subpackage resource name %q invalid: %s", subpackageName, strings.Join(objNameErrs, ","))
112151
}
113152
}
114153

@@ -130,28 +169,6 @@ func getSubpackageDir(task Task) string {
130169
}
131170
}
132171

133-
// IsValidSubpackageDir returns true if subpackageDir is valid, false otherwise.
134-
func IsValidSubpackageDir(subpackageDir string) bool {
135-
// Empty string is invalid, a subpackage directory must be a relative path.
136-
if subpackageDir == "" {
137-
return false
138-
}
139-
140-
// Check basic format and ensure it doesn't contain '..' or start with '/' or end with '/'
141-
if subpackageDir[0] == '/' || strings.HasSuffix(subpackageDir, "/") || noDoubleDots.MatchString(subpackageDir) {
142-
return false
143-
}
144-
145-
// Reject any path segment equal to "." (for example ".", "./subpkg", or "subpkg/./nested").
146-
for _, segment := range strings.Split(subpackageDir, "/") {
147-
if segment == "." {
148-
return false
149-
}
150-
}
151-
152-
return validRelativePathRegex.MatchString(subpackageDir)
153-
}
154-
155172
func (pr *PackageRevision) IsPushOnRenderFailure() bool {
156173
ann := pr.GetAnnotations()
157174
v, ok := ann[PushOnFnRenderFailureKey]

api/porch/v1alpha1/util_test.go

Lines changed: 38 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -23,45 +23,52 @@ import (
2323

2424
func TestIsValidSubpackageDir(t *testing.T) {
2525
tests := []struct {
26-
name string
27-
dir string
28-
expected bool
26+
name string
27+
dir string
28+
expectValid bool
2929
}{
3030
// Invalid cases
31-
{name: "empty string", dir: "", expected: false},
32-
{name: "leading slash", dir: "/subpkg", expected: false},
33-
{name: "trailing slash", dir: "subpkg/", expected: false},
34-
{name: "double dots at start", dir: "../subpkg", expected: false},
35-
{name: "double dots in middle", dir: "sub/../pkg", expected: false},
36-
{name: "double dots at end", dir: "subpkg/..", expected: false},
37-
{name: "only double dots", dir: "..", expected: false},
38-
{name: "dot segment at start", dir: "./subpkg", expected: false},
39-
{name: "dot segment in middle", dir: "sub/./pkg", expected: false},
40-
{name: "only dot", dir: ".", expected: false},
41-
{name: "leading and trailing slash", dir: "/subpkg/", expected: false},
42-
{name: "spaces in path", dir: "sub pkg", expected: false},
43-
{name: "special characters", dir: "sub@pkg", expected: false},
44-
{name: "backslash", dir: "sub\\pkg", expected: false},
45-
{name: "colon in path", dir: "sub:pkg", expected: false},
46-
{name: "empty segment (double slash)", dir: "sub//pkg", expected: false},
31+
{name: "empty string", dir: "", expectValid: false},
32+
{name: "leading slash", dir: "/subpkg", expectValid: false},
33+
{name: "trailing slash", dir: "subpkg/", expectValid: false},
34+
{name: "double dots at start", dir: "../subpkg", expectValid: false},
35+
{name: "double dots in middle", dir: "sub/../pkg", expectValid: false},
36+
{name: "double dots at end", dir: "subpkg/..", expectValid: false},
37+
{name: "only double dots", dir: "..", expectValid: false},
38+
{name: "dot segment at start", dir: "./subpkg", expectValid: false},
39+
{name: "dot segment in middle", dir: "sub/./pkg", expectValid: false},
40+
{name: "only dot", dir: ".", expectValid: false},
41+
{name: "leading and trailing slash", dir: "/subpkg/", expectValid: false},
42+
{name: "spaces in path", dir: "sub pkg", expectValid: false},
43+
{name: "special characters", dir: "sub@pkg", expectValid: false},
44+
{name: "backslash", dir: "sub\\pkg", expectValid: false},
45+
{name: "colon in path", dir: "sub:pkg", expectValid: false},
46+
{name: "empty segment (double slash)", dir: "sub//pkg", expectValid: false},
47+
{name: "with underscores (invalid DNS)", dir: "my_subpkg", expectValid: false},
48+
{name: "mixed with underscores (invalid DNS)", dir: "my-sub_pkg.v1/nested-dir", expectValid: false},
49+
{name: "with dots in name", dir: "my.subpkg", expectValid: false},
4750

4851
// Valid cases
49-
{name: "simple directory", dir: "subpkg", expected: true},
50-
{name: "nested directory", dir: "path/to/subpkg", expected: true},
51-
{name: "two levels", dir: "sub/pkg", expected: true},
52-
{name: "with hyphens", dir: "my-subpkg", expected: true},
53-
{name: "with underscores", dir: "my_subpkg", expected: true},
54-
{name: "with dots in name", dir: "my.subpkg", expected: true},
55-
{name: "numeric name", dir: "123", expected: true},
56-
{name: "mixed valid chars", dir: "my-sub_pkg.v1/nested-dir", expected: true},
57-
{name: "deeply nested", dir: "a/b/c/d/e", expected: true},
58-
{name: "single char segments", dir: "a/b/c", expected: true},
52+
{name: "simple directory", dir: "subpkg", expectValid: true},
53+
{name: "nested directory", dir: "path/to/subpkg", expectValid: true},
54+
{name: "two levels", dir: "sub/pkg", expectValid: true},
55+
{name: "with hyphens", dir: "my-subpkg", expectValid: true},
56+
{name: "numeric name", dir: "123", expectValid: true},
57+
{name: "deeply nested", dir: "a/b/c/d/e", expectValid: true},
58+
{name: "single char segments", dir: "a/b/c", expectValid: true},
59+
{name: "starts with digit", dir: "1subpackage", expectValid: true},
60+
{name: "ends with digit", dir: "subpackage1", expectValid: true},
61+
{name: "contains digits", dir: "1subpckage2/3subpackage4/5subpackage6", expectValid: true},
5962
}
6063

6164
for _, tt := range tests {
6265
t.Run(tt.name, func(t *testing.T) {
63-
result := IsValidSubpackageDir(tt.dir)
64-
assert.Equal(t, tt.expected, result)
66+
err := IsValidSubpackageDir(tt.dir)
67+
if tt.expectValid {
68+
assert.NoError(t, err)
69+
} else {
70+
assert.Error(t, err)
71+
}
6572
})
6673
}
6774
}

docs/content/en/docs/2_concepts/subpackages.md

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,8 @@ parent package revision. The clone operation:
4040

4141
1. Copies the upstream package contents into the specified subdirectory
4242
2. Preserves the upstream's `Kptfile`
43-
3. Adds the origin information of the subpackage to the `Kptfile` of the cloned subpackage
43+
3. Updates the `metadata.name` of the subpackage
44+
4. Adds the origin information of the subpackage to the `Kptfile` of the cloned subpackage
4445

4546
```bash
4647
porchctl rpkg clone upstream-repo.blueprint.v1 deployment.my-app.v2 \
@@ -57,9 +58,10 @@ When a new version of the upstream package is published, the independent subpack
5758
the parent package. The upgrade operation:
5859

5960
1. Reads the subpackage's `Kptfile` to determine its current upstream source
60-
2. Merges the new upstream version into the subpackage directory employing the specified strategy,
61-
using the same mechanism as a regular package revision upgrade
62-
3. Updates the origin information of the subpackage in the `Kptfile` of the cloned subpackage
61+
2. Merges the new upstream version into the subpackage directory employing the specified strategy, using the same mechanism
62+
as a regular package revision upgrade
63+
3. Updates the `metadata.name` of the subpackage
64+
4. Updates the origin information of the subpackage in the `Kptfile` of the cloned subpackage
6365

6466
```bash
6567
porchctl rpkg upgrade deployment.my-app.v2 \
@@ -77,7 +79,7 @@ one after another before it is proposed and approved.
7779
## Constraints
7880

7981
- The parent package revision must be in **Draft** state for both clone and upgrade operations
80-
- The `--subpackage-dir` path must be a valid relative path (no leading `/`, `./`, or `..` segments)
82+
- The `--subpackage-dir` path must be a valid relative path (no leading `/`, `./`, `.`, or `..` segments) and comply with the subpackage directory naming rules described in the [subpackage naming](#subpackage-naming) section below.
8183
- For clone: the subdirectory must **not already exist** in the package
8284
- For upgrade: the subdirectory **must already exist** and contain a valid `Kptfile` with upstream information
8385
- `--workspace` and `--repository` must not be specified when using `--subpackage-dir`
@@ -91,6 +93,36 @@ one after another before it is proposed and approved.
9193
| Upgrade | Creates a new package revision | Modifies the parent package revision in-place |
9294
| Tracking | PackageRevision tracks upstream | Subpackage's Kptfile tracks upstream |
9395

96+
## Subpackage Naming
97+
98+
When Porch clones or upgrades a subpackage it names the subpackage (sets `metadata.name`) based on the `--subpackage-dir` parameter value (`subpackageDir` on the API).
99+
It creates a Kubernetes-compliant DNS subdomain name and inserts it in the `metadata.name` field of the Kptfile.
100+
101+
Porch converts any “/“ characters in the `--subpackage-dir` (or `subpackageDir` on the API) value into ‘.’ characters to create a
102+
[valid Kubernetes DNS Subdomain name](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). This creates a
103+
unique name for the subpackage in the package. This means that `--subpackage-dir` and `subpackageDir` values are restricted to the rules for subdomain
104+
names in `--subpackage-dir` and `subpackageDir` values.
105+
106+
- No more than 253 characters (after replacing "/" with ".")
107+
- Only lowercase alphanumeric characters, "-", "/" ("/" is converted to ".")
108+
- Must start and end with an alphanumeric character (letter or digit)
109+
110+
This [Kubernetes validation IsDNS1123Subdomain() function](https://github.com/kubernetes/apimachinery/blob/master/pkg/util/validation/validation.go)
111+
is used to check the value once "/" characters are replaced with "." characters.
112+
113+
So the following `subpackageDir` values result in the following `metadata.name` values in the Kptfile:
114+
115+
| subpackageDir | metadata.name |
116+
|------------------------------------------|------------------------------------------|
117+
| subpackage | subpackage |
118+
| ran/subpackage | ran.subpackage |
119+
| ran/south/southeast/region-1a/subpackage | ran.south.southeast.region-1a.subpackage |
120+
| 1subpackage | 1subpackage |
121+
| 1subpckage2/3subpackage4/5subpackage6 | 1subpckage2.3subpackage4.5subpackage6 |
122+
| Subpackage | error (Uppercase character) |
123+
| sub_package | error ("_" illegal) |
124+
| ran\subpackage | error ("\\" illegal) |
125+
94126
## Key Points
95127

96128
- Independent subpackages enable composing a package from multiple upstream sources

docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/working-with-subpackages.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ Unlike a regular clone (which creates a new package revision), a subpackage clon
2424
Draft package revision. Similarly, a subpackage upgrade modifies the parent package revision in-place rather than
2525
creating a new one.
2626

27+
Note that **Subpackage directory** paths must follow the
28+
[rules described on the subpackage page]({{% relref "/docs/2_concepts/subpackages/#subpackage-naming" %}}).
29+
2730
## Prerequisites
2831

2932
Before following this guide, ensure you have:

docs/content/en/docs/7_cli_api/porchctl.md

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,13 +327,17 @@ porchctl rpkg clone SOURCE_PACKAGE NAME [flags]
327327
| Flag | Description | Default |
328328
|------|-------------|---------|
329329
| `--repository string` | Downstream repository for cloned package | (required unless `--subpackage-dir` is set) |
330-
| `--workspace string` | Workspace name for new package | `v1` (ignored when `--subpackage-dir` is set; must not be explicitly specified) |
330+
| `--workspace string` | Workspace name for new package | `v1` (must not be explicitly specified together with `--subpackage-dir`) |
331331
| `--directory string` | Directory within upstream repository (Git only) | |
332332
| `--ref string` | Branch, tag, or SHA in upstream repository (Git only) | |
333333
| `--strategy string` | Update strategy: `resource-merge`, `fast-forward`, `force-delete-replace`, `copy-merge` | `resource-merge` |
334334
| `--secret-ref string` | Secret name for basic auth (Git only) | |
335335
| `--subpackage-dir string` | Directory path into which the upstream package will be cloned as an independent subpackage. When set, `NAME` refers to the parent package revision (which must be in Draft state), and `--repository`/`--workspace` must not be specified. | |
336336

337+
Note that **--subpackage-dir** paths must follow the
338+
[rules described on the subpackage page]({{% relref "/docs/2_concepts/subpackages/#subpackage-naming" %}}).
339+
340+
337341
**Examples:**
338342

339343
```bash
@@ -597,6 +601,10 @@ porchctl rpkg upgrade SOURCE_PACKAGE_REVISION [flags]
597601
| `--discover string` | Discover available updates instead of upgrading. Options: `upstream`, `downstream` | |
598602
| `--subpackage-dir string` | Directory path of an independent subpackage to upgrade within the parent package. When set, `SOURCE_PACKAGE_REVISION` refers to the parent Draft package revision, and `--workspace` must not be specified. | |
599603

604+
605+
Note that **--subpackage-dir** paths must follow the
606+
[rules described on the subpackage page]({{% relref "/docs/2_concepts/subpackages/#subpackage-naming" %}}).
607+
600608
**Examples:**
601609

602610
```bash

docs/content/en/docs/9_troubleshooting_and_faq/lazy-dog/_index.md

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,15 @@ into your Starlark script, which will cause an error and trigger the output:
5858
You can use the Porch Kubernetes API directly (via `kubectl` or `curl`) to clone an upstream package as an independent
5959
subpackage into an existing Draft package revision, and to upgrade that subpackage later.
6060

61+
Note that **subpackageDir** paths must follow the
62+
[rules described on the subpackage page]({{% relref "/docs/2_concepts/subpackages/#subpackage-naming" %}}).
63+
6164
### Cloning a subpackage via the API
6265

63-
To clone an upstream package into a subdirectory of an existing Draft package revision, update the parent `PackageRevision` by appending
64-
an additional task of type `clone` that includes `subpackageDir` (for example via `kubectl apply` / server-side apply, or
65-
a PATCH request). The parent must already exist in Draft state with exactly one task.
66+
To clone an upstream package into a subdirectory of an existing Draft package revision, update the parent `PackageRevision` by appending
67+
an additional task of type `clone` that includes `subpackageDir` (for example via `kubectl apply` / server-side apply, or
68+
a PATCH request). The parent must already exist in Draft state with exactly one task.
69+
6670

6771
```json
6872
{
@@ -107,9 +111,9 @@ Key points:
107111

108112
### Upgrading a subpackage via the API
109113

110-
To upgrade an existing independent subpackage, update the parent `PackageRevision` by appending
111-
an additional task of type `upgrade` that includes `subpackageDir` (for example via `kubectl apply` / server-side apply,
112-
or a PATCH request). The parent must be in Draft state with exactly one task.
114+
To upgrade an existing independent subpackage, update the parent `PackageRevision` by appending
115+
an additional task of type `upgrade` that includes `subpackageDir` (for example via `kubectl apply` / server-side apply,
116+
or a PATCH request). The parent must be in Draft state with exactly one task.
113117

114118
```json
115119
{

pkg/cli/commands/rpkg/clone/command.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
porchapi "github.com/kptdev/porch/api/porch/v1alpha1"
2525
cliutils "github.com/kptdev/porch/internal/cliutils"
2626
"github.com/kptdev/porch/pkg/cli/commands/rpkg/docs"
27+
pkgerrors "github.com/pkg/errors"
2728
"github.com/spf13/cobra"
2829
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2930
"k8s.io/apimachinery/pkg/types"
@@ -108,8 +109,8 @@ func (r *runner) preRunE(_ *cobra.Command, args []string) error {
108109
}
109110

110111
} else {
111-
if !porchapi.IsValidSubpackageDir(r.subpackageDir) {
112-
return errors.E(op, fmt.Errorf("invalid --subpackage-dir %q", r.subpackageDir))
112+
if err = porchapi.IsValidSubpackageDir(r.subpackageDir); err != nil {
113+
return errors.E(op, pkgerrors.Wrapf(err, "invalid --subpackage-dir %q", r.subpackageDir))
113114
}
114115

115116
r.clone.SubpackageDir = r.subpackageDir

pkg/cli/commands/rpkg/upgrade/command.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,8 @@ func (r *runner) preRunE(_ *cobra.Command, args []string) error {
122122
return errors.E(op, fmt.Errorf("workspace is required"))
123123
}
124124
} else {
125-
if !porchapi.IsValidSubpackageDir(r.subpackageDir) {
126-
return errors.E(op, fmt.Errorf("invalid --subpackage-dir %q", r.subpackageDir))
125+
if err = porchapi.IsValidSubpackageDir(r.subpackageDir); err != nil {
126+
return errors.E(op, pkgerrors.Wrapf(err, "invalid --subpackage-dir %q", r.subpackageDir))
127127
}
128128

129129
if r.Command.Flags().Changed("workspace") {

pkg/cli/commands/rpkg/upgrade/command_test.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,20 @@ func TestPreRunSubpackageDir(t *testing.T) {
220220
err := r.preRunE(r.Command, []string{"some-pr"})
221221
assert.NoError(t, err)
222222
})
223+
224+
t.Run("Subpackage upgrade with invalid subpackage-dir returns error", func(t *testing.T) {
225+
r := &runner{
226+
ctx: context.Background(),
227+
cfg: &genericclioptions.ConfigFlags{Namespace: func() *string { s := ns; return &s }()},
228+
Command: NewCommand(context.Background(), &genericclioptions.ConfigFlags{Namespace: func() *string { s := ns; return &s }()}),
229+
revision: 2,
230+
subpackageDir: "../invalid",
231+
strategy: "resource-merge",
232+
}
233+
err := r.preRunE(r.Command, []string{"some-pr"})
234+
assert.Error(t, err)
235+
assert.Contains(t, err.Error(), `invalid --subpackage-dir "../invalid"`)
236+
})
223237
}
224238

225239
func TestUpgradeCommand(t *testing.T) {

pkg/task/generictaskhandler.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -277,12 +277,13 @@ func (th *genericTaskHandler) applySubpackageTask(
277277
return err
278278
}
279279

280-
if err := kptFile.SetName(path.Base(subpackageDir)); err != nil {
281-
return pkgerrors.Wrapf(err, "failed to write package name %q to subpackage Kptfile", path.Base(subpackageDir))
280+
subpackageName, _ := porchapi.ComposeSubpkgObjName(subpackageDir)
281+
if err := kptFile.SetName(subpackageName); err != nil {
282+
return pkgerrors.Wrapf(err, "failed to write package name %q to subpackage Kptfile", subpackageName)
282283
}
283284

284285
if err := kptFile.WriteToPackage(subpackageResources.Contents); err != nil {
285-
return pkgerrors.Wrap(err, "failed to write to subpackage Kptfile")
286+
return pkgerrors.Wrapf(err, "failed to write to subpackage Kptfile %q", path.Join(subpackageDir, kptfilev1.KptFileName))
286287
}
287288

288289
// Remove the subpackage task to prevent re-execution of the task

0 commit comments

Comments
 (0)