Add changes to validate name and generateName for revision template by savitaashture · Pull Request #5110 · knative/serving

savitaashture · 2019-08-09T06:29:49Z

knative-prow-robot

Details

In response to this:

Fixes #5094

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.

knative-prow-robot · 2019-08-09T06:30:04Z

Hi @savitaashture. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

markusthoemmes

Looks okay in general, though I'm not the right person to review API nuances. The additional generateName stuff makes me wonder though if we even accept that today?

/assign @mattmoor @dgerd

markusthoemmes · 2019-08-09T06:45:17Z

+
+// ValidateRevisionName validates name and generateName for the revisionTemplate
+func ValidateRevisionName(ctx context.Context, name, generateName string) *apis.FieldError {
+	if generateName != "" {


Did we allow to use generateName before?

If we provide valid or invalid generateName in service spec it will override in the generateName when it creates revision

But if at all user give invalid generateName something like below

apiVersion: serving.knative.dev/v1alpha1 kind: Service metadata: name: "hello" spec: template: metadata: generateName: hi@ spec: containers: - image: savita3020/helloworld

user will not be notified and service will be created successfully by overriding generateName in revision as shown below

kubectl get rev -n test hello-kphn8 -o yaml

apiVersion: serving.knative.dev/v1alpha1 kind: Revision metadata: annotations: serving.knative.dev/creator: kube:admin serving.knative.dev/lastPinned: "1565333856" creationTimestamp: "2019-08-09T06:57:24Z" generateName: hello- generation: 1 labels: serving.knative.dev/configuration: hello serving.knative.dev/configurationGeneration: "1" serving.knative.dev/route: hello serving.knative.dev/service: hello name: hello-kphn8

To just to make sure i have added condition check.

markusthoemmes · 2019-08-09T06:50:00Z

/ok-to-test

vagababov · 2019-08-09T07:49:32Z

+// ValidateRevisionName validates name and generateName for the revisionTemplate
+func ValidateRevisionName(ctx context.Context, name, generateName string) *apis.FieldError {
+	if generateName != "" {
+		msgs := validation.NameIsDNS1035Label(generateName, true)


if msg :=. ...; msg != "" {

type of msgs is []string so we cannot do msgs != ""

Added if condition as per suggestion

vagababov · 2019-08-09T07:52:30Z

+	if generateName != "" {
+		msgs := validation.NameIsDNS1035Label(generateName, true)
+		if len(msgs) > 0 {
+			return &apis.FieldError{


errinvalidvalue?

taragu · 2019-08-12T14:01:35Z

+		want: apis.ErrInvalidValue("not a DNS 1035 label: [a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name',  or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')]",
+			"spec.revisionTemplate.metadata.name"),
+	}, {
+		name: "invalid generate name for configuration spec",


Can we add a valid generate name test case here?

taragu · 2019-08-12T14:57:23Z

+		want: apis.ErrInvalidValue("not a DNS 1035 label: [a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name',  or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')]",
+			"metadata.name"),
+	}, {
+		name: "invalid generate name for revision template",


Same as above. Can we add a valid generate name test case here?

@taragu In revision even if user provide generate name in spec it will be overriden so just added testcase for invalid generate name

savitaashture · 2019-09-06T06:30:55Z

@dgerd this PR is ready for another review. Would you please take a look?

knative-test-reporter-robot · 2019-09-18T17:18:49Z

The following jobs failed due to test flakiness:

Test name	Triggers	Retries
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/apis/serving/v1.TestConfigurationValidation
pkg/apis/serving/v1.TestConfigurationValidation/valid_BYO_name_(with_generateName)

dgerd · 2019-10-23T17:50:56Z

@savitaashture Can you rebase this and I will take a look? This would be good to land in 0.10.x

savitaashture · 2019-10-24T07:35:48Z

@dgerd Rebased
Please have a look :)

knative-metrics-robot · 2019-10-24T07:39:52Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/metadata_validation.go	97.6%	98.2%	0.6
pkg/apis/serving/v1/revision_validation.go	95.2%	94.4%	-0.8
pkg/apis/serving/v1alpha1/revision_validation.go	91.4%	96.1%	4.7

dgerd · 2019-10-28T23:52:07Z

/lgtm
/approve

knative-prow-robot · 2019-10-28T23:52:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgerd, savitaashture

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [dgerd]

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

markusthoemmes · 2019-10-29T07:21:41Z

/test pull-knative-serving-build-tests

markusthoemmes · 2019-10-29T10:03:19Z

/test pull-knative-serving-build-tests

savitaashture · 2019-11-11T07:33:24Z

/retest

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 9, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2019

knative-prow-robot requested review from evankanderson and vaikas August 9, 2019 06:29

knative-prow-robot reviewed Aug 9, 2019

View reviewed changes

Comment thread pkg/apis/serving/metadata_validation.go

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 9, 2019

knative-prow-robot added the area/API API objects and controllers label Aug 9, 2019

savitaashture force-pushed the issue-5094 branch 2 times, most recently from 4a94098 to 3bd86ee Compare August 9, 2019 06:35

markusthoemmes reviewed Aug 9, 2019

View reviewed changes

knative-prow-robot assigned dgerd and mattmoor Aug 9, 2019

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 9, 2019

vagababov reviewed Aug 9, 2019

View reviewed changes

savitaashture force-pushed the issue-5094 branch from 3bd86ee to 99a478f Compare August 9, 2019 09:15

mattmoor added area/API API objects and controllers and removed area/API API objects and controllers labels Aug 9, 2019

taragu reviewed Aug 12, 2019

View reviewed changes

savitaashture force-pushed the issue-5094 branch from 99a478f to f708b21 Compare August 13, 2019 12:07

mattmoor removed their assignment Aug 13, 2019

savitaashture force-pushed the issue-5094 branch 2 times, most recently from 49d6dd7 to 6c7a439 Compare August 26, 2019 07:58

savitaashture force-pushed the issue-5094 branch from 6c7a439 to c868670 Compare August 28, 2019 04:57

savitaashture force-pushed the issue-5094 branch from 76386a2 to e8dbbfb Compare September 18, 2019 17:12

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 18, 2019

savitaashture force-pushed the issue-5094 branch from e8dbbfb to e5aa699 Compare September 18, 2019 18:08

savitaashture added 6 commits October 24, 2019 12:31

Add changes to validate name and generateName for revision template

37806f3

Fix review comment

7f83cf8

Fix review comment

ae41cc7

Added testcase for v1alpha1/revision_validation.go

c4d7ccd

Rebase

2b4c297

fix conflict issue

8ccdcae

savitaashture force-pushed the issue-5094 branch from e5aa699 to 8ccdcae Compare October 24, 2019 07:34

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2019

knative-prow-robot merged commit 2a1f3b9 into knative:master Nov 11, 2019

Conversation

savitaashture commented Aug 9, 2019

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knative-prow-robot commented Aug 9, 2019

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Aug 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savitaashture Aug 21, 2019 • 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!

savitaashture commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-test-reporter-robot commented Sep 18, 2019

Uh oh!

dgerd commented Oct 23, 2019

Uh oh!

savitaashture commented Oct 24, 2019

Uh oh!

knative-metrics-robot commented Oct 24, 2019

Uh oh!

dgerd commented Oct 28, 2019

Uh oh!

knative-prow-robot commented Oct 28, 2019

Uh oh!

markusthoemmes commented Oct 29, 2019

Uh oh!

markusthoemmes commented Oct 29, 2019

Uh oh!

savitaashture commented Nov 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

savitaashture Aug 21, 2019 •

edited

Loading

savitaashture commented Sep 6, 2019 •

edited

Loading