plugins: validate project and plugin versions #1380

estroz · 2020-02-21T00:35:02Z

This PR implements plugin version format enforcement. See the plugins design doc and #1290 for more details.

Changes:

pkg/cli: validate cli struct's fields
pkg/plugin: validation for plugin version strings
pkg/internal/validation: project-wide validation logic; move pkg/model/resource/validation.go functions here

/cc @DirectXMan12 @mengqiy @hasbro17 @Adirio

k8s-ci-robot · 2020-02-21T00:35:06Z

@estroz: GitHub didn't allow me to request PR reviews from the following users: Adirio.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

This PR implements plugin version format enforcement. See the plugins design doc and #1290 for more details.

Changes:

pkg/cli: validate cli struct's fields

pkg/plugin: validation for plugin version strings

pkg/model/validation: project-wide validation logic; move pkg/model/resource/validation.go functions here

/cc @DirectXMan12 @mengqiy @hasbro17 @Adirio

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/cli/cli.go

camilamacedo86

Hi @estroz,

Great 👍 . Just an optional nit and I think would be nice add some tests.
Otherwise, it shows

/lgtm

For me.

pkg/model/validation/validation.go

camilamacedo86

/lgtm when pass in the lint.

Adirio

LGTM

Adirio

LGTM

pkg/cli/cli.go

pkg/model/resource/options.go

camilamacedo86 · 2020-02-23T18:58:07Z

/lgtm

estroz · 2020-02-25T18:51:24Z

pkg/internal/validation/dns.go

+	dns1035LabelFmt     string = "[a-z]([-a-z0-9]*[a-z0-9])?"
+)
+
+type dnsValidationConfig struct {


@Adirio @camilamacedo86 refactored these for convenience after rebasing onto the feature branch, PTAL.

I do like the refactor you did, avoiding code duplications through the use of the dnsValidationConfig struct and its check method. A couple of considerations:

When I read IsDNS1XXXLabel I instantly imagine a func (string) bool signature. As the different errors need to be differentiate, we can not change the function signature to that one, so what about changing the name? Maybe around MustBe... or ConformsTo.... I guess we could check how the standard library

Returning a []string containing the errors doesn't seem very idiomatic in Go. Maybe we should just return an error and have the user solve the problem in two steps in case he encounters two errors, as the second one will not be reported until the first one is solved.

Would it make sense to group functions for the same specification? Something in the line of validation.DNS1XXX.ConformsToLabel(myString). You would have either to export the struct instance or an interface to it:

type DNSValidator interface { ConformsToLabel(string) error ConformsToSubdomain(string) error } var ( DNS1023 DNSValidator = dns1023Config DNS1135 DNSValidator = dns1135Config )

They can slo be left for further PRs if you prefer, they are just ideas.

The naming and return values could use some work. Doing this in a follow-up sounds good to me.

pkg/plugin: validation for plugin version strings pkg/internal/validation: project-wide validation logic; move pkg/model/resource/validation.go functions here

Adirio · 2020-02-26T07:26:01Z

pkg/internal/validation/dns.go

+
+var dns1123LabelConfig = dnsValidationConfig{
+	format:   dns1123LabelFmt,
+	maxLen:   56, // = 63 - len("-system")


Non-blocking: I think the validation package should not know about the "-system" prefix, the method should be called with that already prefixed and the maxLen field should be left to 63.

Just to clarifies: The project name will be used as part of the namespace added in the files.
So, it will be <projectname-system>. In this way, when we scaffold the project we cannot allow a projectName with more than 56 characters. However, I agree that how it will be implemented as a hall for improvements like you described.

However, it may not part of the context of this PR as well.

DirectXMan12 · 2020-02-26T23:12:56Z

/approve

k8s-ci-robot · 2020-02-26T23:13:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, estroz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Adirio · 2020-02-26T23:15:24Z

An ok-to-test should be applied for prow tests, right?

mengqiy · 2020-02-27T00:42:54Z

/ok-to-test

camilamacedo86

/lgtm

k8s-ci-robot requested review from DirectXMan12 and mengqiy February 21, 2020 00:35

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 21, 2020

k8s-ci-robot requested a review from hasbro17 February 21, 2020 00:35

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 21, 2020

camilamacedo86 reviewed Feb 21, 2020

View reviewed changes

pkg/cli/cli.go Show resolved Hide resolved

camilamacedo86 reviewed Feb 21, 2020

View reviewed changes

pkg/cli/cli.go Show resolved Hide resolved

camilamacedo86 reviewed Feb 21, 2020

View reviewed changes

pkg/cli/cli.go Show resolved Hide resolved

camilamacedo86 approved these changes Feb 21, 2020

View reviewed changes

k8s-ci-robot assigned camilamacedo86 Feb 21, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2020

Adirio suggested changes Feb 21, 2020

View reviewed changes

pkg/model/validation/validation.go Outdated Show resolved Hide resolved

estroz force-pushed the feature/plugins-enforce-version-format branch from bcb985d to d7aaa06 Compare February 21, 2020 18:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2020

estroz force-pushed the feature/plugins-enforce-version-format branch from d7aaa06 to ffa2875 Compare February 21, 2020 18:59

camilamacedo86 approved these changes Feb 21, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2020

Adirio approved these changes Feb 22, 2020

View reviewed changes

pkg/cli/cli.go Show resolved Hide resolved

pkg/model/resource/options.go Show resolved Hide resolved

estroz force-pushed the feature/plugins-enforce-version-format branch from ffa2875 to 9485888 Compare February 22, 2020 21:29

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 23, 2020

estroz force-pushed the feature/plugins-enforce-version-format branch from 9485888 to 5215a49 Compare February 25, 2020 18:49

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 25, 2020

estroz commented Feb 25, 2020

View reviewed changes

pkg/cli: validate cli struct's fields

b3d7e23

pkg/plugin: validation for plugin version strings pkg/internal/validation: project-wide validation logic; move pkg/model/resource/validation.go functions here

estroz force-pushed the feature/plugins-enforce-version-format branch from 5215a49 to b3d7e23 Compare February 26, 2020 00:20

Adirio reviewed Feb 26, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2020

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 27, 2020

camilamacedo86 reviewed Feb 27, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2020

k8s-ci-robot merged commit c1f48d0 into kubernetes-sigs:feature/plugins-part-2-electric-boogaloo Feb 27, 2020

estroz deleted the feature/plugins-enforce-version-format branch February 27, 2020 17:59

plugins: validate project and plugin versions #1380

plugins: validate project and plugin versions #1380

Uh oh!

Conversation

estroz commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

camilamacedo86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adirio left a comment

Choose a reason for hiding this comment

Uh oh!

Adirio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 commented Feb 23, 2020

Uh oh!

estroz Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

Adirio Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

Adirio Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 commented Feb 26, 2020

Uh oh!

k8s-ci-robot commented Feb 26, 2020

Uh oh!

Adirio commented Feb 26, 2020

Uh oh!

mengqiy commented Feb 27, 2020

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

estroz commented Feb 21, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

Adirio Feb 26, 2020 •

edited

Loading

Adirio Feb 26, 2020 •

edited

Loading

camilamacedo86 Feb 26, 2020 •

edited

Loading