Skip to content

Commit

Permalink
Review suggestions
Browse files Browse the repository at this point in the history
- Split regex for first (os and version) and further components (arch, variant)
- Update regular expression to preserve the existing set of accepted characters
  for the OS part of the first element (avoid accepting ".", ")", "(").
- Make regular expression for osVersion more strict, and only accept dot-separated
  numbers.
- Add capture groups to the osAndVersion regular expression so that the result
  can be consumed directly, without having to split after validating.
- Now that we already separate OS asnd OSVersion ahead; simplify normalizeOS
  to only normalize the OS.
- Remove the OSAndVersionFormat variable, and inline it.
- Update Parse() to prevent un-bounded string splitting.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
  • Loading branch information
thaJeztah committed May 14, 2024
1 parent c1438e9 commit a942ac7
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 11 deletions.
2 changes: 2 additions & 0 deletions database.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ func isKnownArch(arch string) bool {
return false
}

// normalizeOS returns the normalized OS. If the os is empty, it returns
// [runtime.GOOS].
func normalizeOS(os string) string {
if os == "" {
return runtime.GOOS
Expand Down
8 changes: 5 additions & 3 deletions platforms.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@
// While the OCI platform specifications provide a tool for components to
// specify structured information, user input typically doesn't need the full
// context and much can be inferred. To solve this problem, we introduced
// "specifiers". A specifier has the format
// `<os>|<arch>|<os>/<arch>[/<variant>]`. The user can provide either the
// operating system or the architecture or both.
// "specifiers". A specifier has the format:
//
// <os>[(<osVersion>)]|<arch>|<os>/<arch>[/<variant>]
//
// The user can provide either the operating system or the architecture or both.
//
// An example of a common specifier is `linux/amd64`. If the host has a default
// of runtime that matches this, the user can simply provide the component that
Expand Down
45 changes: 37 additions & 8 deletions platforms_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,26 @@ func TestParseSelector(t *testing.T) {
formatted: path.Join("windows(10.0.17763)", defaultArch, defaultVariant),
useV2Format: true,
},
{
input: "windows(10.0.17763)/amd64",
expected: specs.Platform{
OS: "windows",
OSVersion: "10.0.17763",
Architecture: "amd64",
},
formatted: "windows(10.0.17763)/amd64",
useV2Format: true,
},
{
input: "macos(Abcd.Efgh.123-4)/aarch64",
expected: specs.Platform{
OS: "darwin",
OSVersion: "Abcd.Efgh.123-4",
Architecture: "arm64",
},
formatted: "darwin(Abcd.Efgh.123-4)/arm64",
useV2Format: true,
},
} {
t.Run(testcase.input, func(t *testing.T) {
if testcase.skip {
Expand Down Expand Up @@ -370,10 +390,10 @@ func TestParseSelector(t *testing.T) {
}

formatted := ""
if testcase.useV2Format == false {
formatted = Format(p)
} else {
if testcase.useV2Format {
formatted = FormatAll(p)
} else {
formatted = Format(p)
}
if formatted != testcase.formatted {
t.Fatalf("unexpected format: %q != %q", formatted, testcase.formatted)
Expand All @@ -385,14 +405,14 @@ func TestParseSelector(t *testing.T) {
t.Fatalf("error parsing formatted output: %v", err)
}

if testcase.useV2Format == false {
if Format(reparsed) != formatted {
t.Fatalf("normalized output did not survive the round trip: %v != %v", Format(reparsed), formatted)
}
} else {
if testcase.useV2Format {
if FormatAll(reparsed) != formatted {
t.Fatalf("normalized output did not survive the round trip: %v != %v", FormatAll(reparsed), formatted)
}
} else {
if Format(reparsed) != formatted {
t.Fatalf("normalized output did not survive the round trip: %v != %v", Format(reparsed), formatted)
}
}
})
}
Expand Down Expand Up @@ -420,6 +440,15 @@ func TestParseSelectorInvalid(t *testing.T) {
{
input: "linux/arm/foo/bar", // too many components
},
{
input: "amd64/windows(10.0.17763)/foo", // only first element accepts os[(osVersion)]
},
{
input: "linux)()---()..../arm/foo",
},
{
input: "../arm/foo",
},
} {
t.Run(testcase.input, func(t *testing.T) {
if _, err := Parse(testcase.input); err == nil {
Expand Down

0 comments on commit a942ac7

Please # to comment.