-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
pkg/plugin/validation.go: wrap errors in types #1541
pkg/plugin/validation.go: wrap errors in types #1541
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: estroz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
} | ||
cv := semver.Version{Major: v.Major, Minor: v.Minor} | ||
if !v.Equals(cv) { | ||
return errInvalidVersion{version, "must contain major and minor version only"} |
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.
The plugin version in the master is v2.0.0
so, if it is working should not be failing in the CI to run the script which generates the testdata?
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.
Good catch, this should be done using string comp.
"fmt" | ||
|
||
"github.com/blang/semver" | ||
|
||
"sigs.k8s.io/kubebuilder/pkg/internal/validation" | ||
) | ||
|
||
// errInvalidVersion should be returned when a plugin's version is invalid. | ||
type errInvalidVersion struct { |
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.
Since we are implementing specific types of error would be good to export them to be used. Also, wdyt about simplified it by:
type errInvalidVersion struct { | |
type ErrInvalidVersion struct { |
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.
These aren't used anywhere else, yet.
return nil | ||
} | ||
|
||
// errInvalidName should be returned when a plugin's name is invalid. | ||
type errInvalidName struct { |
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.
type errInvalidName struct { | |
type ErrInvalidName struct { |
type errInvalidVersion struct { | ||
version, msg string | ||
} | ||
|
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.
also, I'd suggest keep the struct definition on top and the methods on the bottom.
if errs := validation.IsDNS1123Subdomain(name); len(errs) != 0 { | ||
return fmt.Errorf("plugin name %q is invalid: %v", name, errs) | ||
return errInvalidName{name, fmt.Sprintf("%s", errs)} | ||
} | ||
return nil | ||
} |
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 think we need to cover it with unit test as well. (validation_test.go)
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.
These are really simple wrappers for errors, I don't thing that is necessary. Also the functions themselves are fairly simple, although I could see unit testing the version validator.
@@ -17,32 +17,64 @@ limitations under the License. | |||
package plugin | |||
|
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.
the name of the file is validation, so should we not centralize here all plugin validations?
Or your idea is just to have the plugin version validations? If yes, could we rename the file to make it clear or centralize all plugin validations?
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.
This file contains functions that let you build a plugin validator by defining what a valid plugin name/version is. I don't see a reason to change anything but error types.
Closing in favor of #1547 |
Plugin validation errors can be abstracted into types.
/cc @camilamacedo86