Skip to content

feat: hints on slices #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
upils wants to merge 9 commits into
base: main
Choose a base branch
from
+81 −4
Open

feat: hints on slices #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
91126a2
feat: support hints
upils Jan 9, 2026
8e16979
test: hint marshalling
upils Jan 9, 2026
5a14e13
refactor: apply PR suggestion
upils Jan 12, 2026
35173f8
style: apply PR suggestions
upils Jan 12, 2026
92ffeb7
style: improve test hint values
upils Jan 12, 2026
3b29f36
fix: forbid all non-standad spaces from hints
upils Jan 13, 2026
aec7d72
fix: display consistent validation errors
upils Jan 13, 2026
93131d9
fix: only allow printable chars and space in hints
upils Jan 13, 2026
56d7085
refactor: apply PR suggestions
upils Jan 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions cmd/chisel/cmd_find.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,13 @@ func (cmd *cmdFind) Execute(args []string) error {
}

w := tabWriter()
fmt.Fprintf(w, "Slice\tSummary\n")
fmt.Fprintf(w, "Slice\tHint\n")
for _, s := range slices {
fmt.Fprintf(w, "%s\t%s\n", s, "-")
hint := "-"
if s.Hint != "" {
hint = s.Hint
}
fmt.Fprintf(w, "%s\t%s\n", s, hint)
}
w.Flush()

Expand Down
7 changes: 7 additions & 0 deletions cmd/chisel/cmd_info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ var infoTests = []infoTest{{
package: mypkg2
slices:
myslice:
hint: Hint for mypkg2_myslice
contents:
/dir/another-file: {}
`,
Expand All @@ -52,6 +53,7 @@ var infoTests = []infoTest{{
contents:
/dir/file: {}
myslice2:
hint: Hint for mypkg1_myslice2
v3-essential:
mypkg1_myslice1: {}
mypkg2_myslice: {arch: amd64}
Expand All @@ -67,13 +69,15 @@ var infoTests = []infoTest{{
contents:
/dir/file: {}
myslice2:
hint: Hint for mypkg1_myslice2
v3-essential:
mypkg1_myslice1: {}
mypkg2_myslice: {arch: amd64}
---
package: mypkg2
slices:
myslice:
hint: Hint for mypkg2_myslice
contents:
/dir/another-file: {}
`,
Expand All @@ -88,6 +92,7 @@ var infoTests = []infoTest{{
contents:
/dir/file: {}
myslice2:
hint: Hint for mypkg1_myslice2
v3-essential:
mypkg1_myslice1: {}
mypkg2_myslice: {arch: amd64}
Expand Down Expand Up @@ -148,13 +153,15 @@ var infoRelease = map[string]string{
contents:
/dir/file:
myslice2:
hint: Hint for mypkg1_myslice2
v3-essential:
mypkg2_myslice: {arch: amd64}
`,
"slices/mypkg2.yaml": `
package: mypkg2
slices:
myslice:
hint: Hint for mypkg2_myslice
contents:
/dir/another-file:
`,
Expand Down
1 change: 1 addition & 0 deletions internal/setup/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ type Package struct {
type Slice struct {
Package string
Name string
Hint string
Essential map[SliceKey]EssentialInfo
Contents map[string]PathInfo
Scripts SliceScripts
Expand Down
57 changes: 56 additions & 1 deletion internal/setup/setup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1265,7 +1265,49 @@ var setupTests = []setupTest{{
/usr/bin/cc:
`,
},
relerror: `invalid slice name "cc" in slices/mydir/mypkg.yaml \(start with a-z, len >= 3, only a-z / 0-9 / -\)`,
relerror: `invalid slice name in slices/mydir/mypkg.yaml \(start with a-z, len >= 3, only a-z / 0-9 / -\): "cc"`,
}, {
summary: "Invalid slice hint - too long",
input: map[string]string{
"slices/mydir/mypkg.yaml": `
package: mypkg
slices:
slice1:
hint: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
contents:
/usr/bin/cc:
`,
},
relerror: `invalid slice hint for "slice1" in slices/mydir/mypkg.yaml \(len <= 40, only printable characters and standard space\): "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"`,
}, {
summary: "Invalid slice hint - line breaks",
input: map[string]string{
"slices/mydir/mypkg.yaml": `
package: mypkg
slices:
slice1:
hint: |
On
multiple
lines.
contents:
/usr/bin/cc:
`,
},
relerror: `invalid slice hint for "slice1" in slices/mydir/mypkg.yaml \(len <= 40, only printable characters and standard space\): "On\\nmultiple\\nlines.\\n"`,
}, {
summary: "Invalid slice hint - non-standard spaces",
Copy link
Collaborator Author

@upils upils Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Note to reviewers]: This test might be a bit redundant but I wanted to test a non-standard space other than a line break.

input: map[string]string{
"slices/mydir/mypkg.yaml": `
package: mypkg
slices:
slice1:
hint: "Seperated\tby\ttabs."
contents:
/usr/bin/cc:
`,
},
relerror: `invalid slice hint for "slice1" in slices/mydir/mypkg.yaml \(len <= 40, only printable characters and standard space\): "Seperated\\tby\\ttabs."`,
}, {
summary: "Package essentials with same package slice",
input: map[string]string{
Expand Down Expand Up @@ -3702,6 +3744,19 @@ func (s *S) TestPackageYAMLFormat(c *C) {
/dir/file: {}
`,
},
}, {
summary: "Slice with hint",
input: map[string]string{
"slices/mypkg.yaml": `
package: mypkg
archive: ubuntu
slices:
myslice:
hint: Shipping greatness
contents:
/dir/file: {}
`,
},
}, {
summary: "All types of paths",
input: map[string]string{
Expand Down
12 changes: 11 additions & 1 deletion internal/setup/yaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"slices"
"strings"
"time"
"unicode"

"golang.org/x/crypto/openpgp/packet"
"gopkg.in/yaml.v3"
Expand Down Expand Up @@ -146,6 +147,7 @@ func (ym yamlMode) MarshalYAML() (any, error) {
var _ yaml.Marshaler = yamlMode(0)

type yamlSlice struct {
Hint string `yaml:"hint,omitempty"`
Essential []string `yaml:"essential,omitempty"`
Contents map[string]*yamlPath `yaml:"contents,omitempty"`
Mutate string `yaml:"mutate,omitempty"`
Expand Down Expand Up @@ -407,11 +409,18 @@ func parsePackage(baseDir, pkgName, pkgPath string, data []byte) (*Package, erro
for sliceName, yamlSlice := range yamlPkg.Slices {
match := apacheutil.SnameExp.FindStringSubmatch(sliceName)
if match == nil {
return nil, fmt.Errorf("invalid slice name %q in %s (start with a-z, len >= 3, only a-z / 0-9 / -)", sliceName, pkgPath)
return nil, fmt.Errorf("invalid slice name in %s (start with a-z, len >= 3, only a-z / 0-9 / -): %q", pkgPath, sliceName)
}
hintNotPrintable := strings.ContainsFunc(yamlSlice.Hint, func(r rune) bool {
return !unicode.IsPrint(r)
})
if len(yamlSlice.Hint) > 40 || hintNotPrintable {
return nil, fmt.Errorf("invalid slice hint for %q in %s (len <= 40, only printable characters and standard space): %q", sliceName, pkgPath, yamlSlice.Hint)
Copy link
Collaborator Author

@upils upils Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Note to reviewers]: The way to list the rules here is inconsistent with the other error message for slice names. I tried to express it the same way but it led to something long and hard to read.
Reworking the other error message also led to something longer and harder to understand.
Both are currently expressed as "inclusion sets" but the one for hints is in practice closer to an "exclusion set", thus making it difficult to actually list everything allowed.

So I propose to compromise a bit on consistency to gain in clarity.

Copy link
Collaborator

@letFunny letFunny Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a balance here to strike. We want to be precise but we do not want to create an overly complex error message that will confuse the user. If you look at what we do for slice names, the message is not 100% coherent with the regex but it is enough for the user to know how to write a good slice name.

We can do a similar thing here, I think "printable characters" is too technical as it refers to a concrete subset of unicode. Also, my assumption is that no user is going to use non-printable characters accidentally. So I think we can simplify the message to what you had before, (len <= 40, only " " allowed).

Copy link
Collaborator Author

@upils upils Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only " " allowed

This, alone, looks misleading to me. We are not only allowing spaces. What about " " only space allowed or only standard space allowed?

}
slice := &Slice{
Package: pkgName,
Name: sliceName,
Hint: yamlSlice.Hint,
Scripts: SliceScripts{
Mutate: yamlSlice.Mutate,
},
Expand Down Expand Up @@ -631,6 +640,7 @@ func pathInfoToYAML(pi *PathInfo) (*yamlPath, error) {
// sliceToYAML converts a Slice object to a yamlSlice object.
func sliceToYAML(s *Slice) (*yamlSlice, error) {
slice := &yamlSlice{
Hint: s.Hint,
Contents: make(map[string]*yamlPath, len(s.Contents)),
Mutate: s.Scripts.Mutate,
V3Essential: make(map[string]*yamlEssential, len(s.Essential)),
Expand Down
Loading