-
Notifications
You must be signed in to change notification settings - Fork 215
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.
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
spec: fix regexp #426
spec: fix regexp #426
Conversation
As pointed out by @andaaron in opencontainers#425, the regular expression proposed there was bogus. It was actually wrong in two different ways. Mea culpa, my apologies. This fixes it. I wrote a few test cases to check: https://go.dev/play/p/4_iYH4Hao1- ```go package main import ( "regexp" "testing" ) func TestRepoPattern(t *testing.T) { r := regexp.MustCompile(`^[a-z0-9]+((\.|_|__|-+)[a-z0-9]+)*(/[a-z0-9]+((\.|_|__|-+)[a-z0-9]+)*)*$`) for _, test := range []struct { s string want bool }{ {"foo", true}, {"foo/bar", true}, {"foo/bar__baz", true}, {"foo/bar___baz", false}, {"/foo", false}, {"foo//bar", false}, {"foo-------b__x.com", true}, {"foo-------b__x.com/p----x", true}, {"foo-", false}, {"-foo", false}, {"foo/-bar", false}, {"foo/bar-", false}, {"foo-bar", true}, {"foo----_bar", false}, {"foo----bar_/x", false}, } { if got, want := r.MatchString(test.s), test.want; got != want { t.Errorf("mismatch on %q; got %v want %v", test.s, got, want) } } } ``` Ideally something like the above would be committed for CI testing but perhaps that's a stage too far. Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no power to approve, but I 100% agree this should be included in code with tests. The regexp doesn't even have to be exported, so long as it's kept in sync with spec.md.
Tests let us catch bugs and regressions, and act as runnable documentation to showcase unusual valid and invalid cases.
I'm not sure when it happened, but it looks like we've relaxed the restrictions from the Docker spec:
|
As pointed out by @andaaron in opencontainers#425, the regular expression proposed there was bogus. It was actually wrong in two different ways. Mea culpa, my apologies. This fixes it. I wrote a few test cases to check: https://go.dev/play/p/4_iYH4Hao1- ```go package main import ( "regexp" "testing" ) func TestRepoPattern(t *testing.T) { r := regexp.MustCompile(`^[a-z0-9]+((\.|_|__|-+)[a-z0-9]+)*(/[a-z0-9]+((\.|_|__|-+)[a-z0-9]+)*)*$`) for _, test := range []struct { s string want bool }{ {"foo", true}, {"foo/bar", true}, {"foo/bar__baz", true}, {"foo/bar___baz", false}, {"/foo", false}, {"foo//bar", false}, {"foo-------b__x.com", true}, {"foo-------b__x.com/p----x", true}, {"foo-", false}, {"-foo", false}, {"foo/-bar", false}, {"foo/bar-", false}, {"foo-bar", true}, {"foo----_bar", false}, {"foo----bar_/x", false}, } { if got, want := r.MatchString(test.s), test.want; got != want { t.Errorf("mismatch on %q; got %v want %v", test.s, got, want) } } } ``` Ideally something like the above would be committed for CI testing but perhaps that's a stage too far. Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
As pointed out by @andaaron in #425, the regular expression proposed there was bogus. It was actually wrong in two different ways. Mea culpa, my apologies.
This fixes it. I wrote a few test cases to check: https://go.dev/play/p/4_iYH4Hao1-
Ideally something like the above would be committed for CI testing but perhaps that's a stage too far.