Skip to content
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

👻 Enhance regex patterns #1745

Merged
merged 1 commit into from
Oct 29, 2020
+21 −21
Merged

👻 Enhance regex patterns #1745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions pkg/internal/validation/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ import (
// "k8s.io/apimachinery" is needed, re-consider whether to add the dependency.

const (
dns1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"
dns1123LabelFmt string = "[a-z0-9](?:[-a-z0-9]*[a-z0-9])?"
dns1123SubdomainFmt string = dns1123LabelFmt + "(\\." + dns1123LabelFmt + ")*"
dns1035LabelFmt string = "[a-z]([-a-z0-9]*[a-z0-9])?"
dns1035LabelFmt string = "[a-z](?:[-a-z0-9]*[a-z0-9])?"
Copy link
Member

Choose a reason for hiding this comment

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

@Adirio,

For we not add ore one dependency to the project we decided to copy and paste the implementation to validate.
We should not change this file unless it was changed in the k8s.io/apimachinery

See:

kubebuilder/pkg/internal/validation/dns.go

Lines 24 to 26 in 1759529

// This file's code was modified from "k8s.io/apimachinery/pkg/util/validation"
// to avoid package dependencies. In case of additional functionality from
// "k8s.io/apimachinery" is needed, re-consider whether to add the dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please revert the changes here?

Copy link
Contributor Author

@Adirio Adirio Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would agree with you if the code was copy-pasted, but it has been already modified. You can check it. So Convering that capture group into a non-capture group tells the future us reading the code that we are not using that capture group to take a substring out of the regex. It also avoid having to allocate some variables but the performance increase will be very small.

Copy link
Member

@camilamacedo86 camilamacedo86 Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Catcher. I would prefer if possible we keep a copy and paste of https://github.com/kubernetes/apimachinery/blob/v0.18.6/pkg/util/validation/validation.go whcih would allow us to move to use the dependency if we wish without breaking changes. However, I will let it up to you and @estroz.

)

type dnsValidationConfig struct {
Expand Down
2 changes: 1 addition & 1 deletion pkg/internal/validation/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
)

// projectVersionFmt defines the project version format from a project config.
const projectVersionFmt string = "[1-9][0-9]*(-(alpha|beta))?"
const projectVersionFmt string = "[1-9][0-9]*(?:-(?:alpha|beta))?"

var projectVersionRe = regexp.MustCompile("^" + projectVersionFmt + "$")

Expand Down
2 changes: 1 addition & 1 deletion pkg/model/resource/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
)

const (
versionPattern = "^v\\d+(alpha\\d+|beta\\d+)?$"
versionPattern = "^v\\d+(?:alpha\\d+|beta\\d+)?$"
groupRequired = "group cannot be empty"
versionRequired = "version cannot be empty"
kindRequired = "kind cannot be empty"
Expand Down
20 changes: 10 additions & 10 deletions pkg/model/resource/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,27 +47,27 @@ var _ = Describe("Resource Options", func() {
options := &Options{Group: "crew", Version: "1", Kind: "FirstMate"}
Expect(options.Validate()).NotTo(Succeed())
Expect(options.Validate().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was 1)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was 1)`))
Copy link
Member

@camilamacedo86 camilamacedo86 Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Could we not use fmt.String and have the regex as a const arg of it?

Copy link
Contributor Author

@Adirio Adirio Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I also thought of that to prevent having it hardcoded a dozen times the thing is that the string is not exported from its package. I think the better approach would be to create an expecific error instead a generic one and test if this yields this kind of error. But I just wanted to do the less changes to get it to pass Travis for now.


options = &Options{Group: "crew", Version: "1beta1", Kind: "FirstMate"}
Expect(options.Validate()).NotTo(Succeed())
Expect(options.Validate().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was 1beta1)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was 1beta1)`))

options = &Options{Group: "crew", Version: "a1beta1", Kind: "FirstMate"}
Expect(options.Validate()).NotTo(Succeed())
Expect(options.Validate().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was a1beta1)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was a1beta1)`))

options = &Options{Group: "crew", Version: "v1beta", Kind: "FirstMate"}
Expect(options.Validate()).NotTo(Succeed())
Expect(options.Validate().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was v1beta)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was v1beta)`))

options = &Options{Group: "crew", Version: "v1beta1alpha1", Kind: "FirstMate"}
Expect(options.Validate()).NotTo(Succeed())
Expect(options.Validate().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was v1beta1alpha1)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was v1beta1alpha1)`))
})

It("should fail if the Kind is not specified", func() {
Expand Down Expand Up @@ -149,27 +149,27 @@ var _ = Describe("Resource Options", func() {
options := &Options{Group: "crew", Version: "1", Kind: "FirstMate"}
Expect(options.ValidateV2()).NotTo(Succeed())
Expect(options.ValidateV2().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was 1)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was 1)`))

options = &Options{Group: "crew", Version: "1beta1", Kind: "FirstMate"}
Expect(options.ValidateV2()).NotTo(Succeed())
Expect(options.ValidateV2().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was 1beta1)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was 1beta1)`))

options = &Options{Group: "crew", Version: "a1beta1", Kind: "FirstMate"}
Expect(options.ValidateV2()).NotTo(Succeed())
Expect(options.ValidateV2().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was a1beta1)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was a1beta1)`))

options = &Options{Group: "crew", Version: "v1beta", Kind: "FirstMate"}
Expect(options.ValidateV2()).NotTo(Succeed())
Expect(options.ValidateV2().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was v1beta)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was v1beta)`))

options = &Options{Group: "crew", Version: "v1beta1alpha1", Kind: "FirstMate"}
Expect(options.ValidateV2()).NotTo(Succeed())
Expect(options.ValidateV2().Error()).To(ContainSubstring(
`version must match ^v\d+(alpha\d+|beta\d+)?$ (was v1beta1alpha1)`))
`version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was v1beta1alpha1)`))
})

It("should fail if the Kind is not specified for V2", func() {
Expand Down
2 changes: 1 addition & 1 deletion pkg/plugin/internal/util/go_version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func fetchAndCheckGoVersion() error {
// checkGoVersion should only ever check if the Go version >= 1.13, since the kubebuilder binary only cares
// that the go binary supports go modules which were stabilized in that version (i.e. in go 1.13) by default
func checkGoVersion(verStr string) error {
goVerRegex := `^go?([0-9]+)\.([0-9]+)([\.0-9A-Za-z\-]+)?$`
goVerRegex := `^go?([0-9]+)\.([0-9]+)[\.0-9A-Za-z\-]*$`
m := regexp.MustCompile(goVerRegex).FindStringSubmatch(verStr)
if m == nil {
return fmt.Errorf("invalid version string")
Expand Down
12 changes: 6 additions & 6 deletions pkg/plugin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ const (
)

// verRe defines the string format of a version.
var verRe = regexp.MustCompile("^(v)?([1-9][0-9]*)(-alpha|-beta)?$")
var verRe = regexp.MustCompile("^v?([1-9][0-9]*)(?:-(alpha|beta))?$")

// Version is a plugin version containing a non-zero integer and an optional stage value
// that if present identifies a version as not stable to some degree.
Expand Down Expand Up @@ -75,15 +75,15 @@ func ParseVersion(version string) (v Version, err error) {
return v, errors.New("plugin version is empty")
}

// A valid version string will have 4 submatches, each of which may be empty: the full string, "v",
// the integer, and the stage suffix. Invalid version strings do not have 4 submatches.
// A valid version string will have 3 submatches, each of which may be empty: the full string,
// the integer, and the stage suffix. Invalid version strings do not have 3 submatches.
submatches := verRe.FindStringSubmatch(version)
if len(submatches) != 4 {
if len(submatches) != 3 {
Adirio marked this conversation as resolved.
Show resolved Hide resolved
return v, fmt.Errorf("version format must match %s", verRe.String())
}

// Parse version number.
versionNumStr := submatches[2]
versionNumStr := submatches[1]
if versionNumStr == "" {
return v, errors.New("version must contain an integer")
}
Expand All @@ -92,7 +92,7 @@ func ParseVersion(version string) (v Version, err error) {
}

// Parse stage suffix, if any.
v.Stage = strings.TrimPrefix(submatches[3], "-")
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
v.Stage = submatches[2]

return v, v.Validate()
}
Expand Down