Add devfile validation #301

maysunfaisal · 2021-01-13T22:19:17Z

What does this PR do?

Implement the consolidated validations documented in https://github.com/devfile/api/blob/master/pkg/validation/validation-rule.md

What issues does this PR fix or reference?

Fixes #151

Is your PR tested? Consider putting some instruction how to test your changes

Yes, new go tests

Docs PR

pkg/validation/commands.go

pkg/validation/projects.go

pkg/validation/validation-rule.md

pkg/validation/projects.go

pkg/validation/commands.go

amisevsk

Generally looks good, a few comments regarding test cases.

pkg/apis/workspaces/v1alpha2/keyed_implementations.go

pkg/validation/commands_test.go

amisevsk · 2021-01-21T19:56:41Z

pkg/validation/events_test.go

+			name: "Case 2: Invalid postStart and postStop events",
+			events: v1alpha2.Events{
+				WorkspaceEvents: v1alpha2.WorkspaceEvents{
+					PostStart: []string{
+						"apply1",
+						"exec2",
+						"compositeExecApply",
+					},
+					PostStop: []string{
+						"apply12",
+						"exec2",
+						"compositeExecApply",
+					},
+					PreStart: []string{
+						"apply12",
+						"exec2",
+						"compositeExecApply",
+					},
+					PreStop: []string{
+						"apply12",
+						"exec2",
+						"compositeExecApply",
+					},
+				},
+			},
+			wantErr: true,
+		},


This is an overly-large test case IMO -- we expect a dozen errors to be returned here, but only check that any error is returned; we could pass this test by e.g. checking only that there are no exec commands in preStart.

amisevsk · 2021-01-21T19:57:44Z

pkg/validation/events_test.go

+		eventType  string
+		eventNames []string
+		wantErr    bool
+		wantErrMsg string


wantErrMsg is unused in test cases.

amisevsk · 2021-01-21T20:07:22Z

pkg/validation/events_test.go

+		{
+			name:      "Case 5: Invalid postStart events - Exec and Composite Mixed Commands",
+			eventType: postStart,
+			eventNames: []string{
+				"exec1",
+				"exec2",
+				"compositeExecApply",
+			},
+			wantErr: true,
+		},
+		{
+			name:      "Case 6: Invalid preStop events - Non-Exec and Composite Exec Commands",
+			eventType: preStop,
+			eventNames: []string{
+				"exec1",
+				"apply2",
+				"compositeOnlyExec",
+			},
+			wantErr: true,
+		},


These tests are unclear to me; could we rename the tests to be more precise in what the expected problem is? E.g.

Case 5: Invalid postStart events - Composite command contains non-exec command Case 6: Invalid preStop events - Non-exec command in events

amisevsk · 2021-01-21T20:15:45Z

pkg/validation/projects_test.go

+			name: "Case 2: Invalid Starter Project",
+			starterProjects: []v1alpha2.StarterProject{
+				generateDummyGithubStarterProject("project1", "origin", map[string]string{"origin": "originremote", "test": "testremote"}),
+				generateDummyGithubStarterProject("project2", "origin", map[string]string{"origin": "originremote", "test": "testremote"}),


This test would be clearer if there was only one starter project.

amisevsk · 2021-01-21T20:21:42Z

pkg/validation/utils_test.go

+		{
+			name:       "Case 3: string with numbers and character",
+			arg:        "12_34",
+			wantResult: false,
+		},


Should add a testcase for arg: "0xff" to catch in case someone tries to use ParseInt instead of Atoi. Note also that underscores in IDs are not allowed by regex.

amisevsk · 2021-01-21T20:26:34Z

pkg/validation/utils.go

-	}
-	return false
+func isInt(str string) (bool, error) {
+	match, err := regexp.MatchString("^[0-9]+$", str)


Two minor issues here:

Regex won't match 10-10; if you want to check that at least one letter exists, just check "[a-zA-Z]"

Avoid returning an error by using
var alphaRegexp = regexp.MustCompile(`[a-zA-Z]`)
at the top of the file.

+1 on point 2, thx

On point 1, I just chatted with Stephanie who wrote the initial doc for validation and it seems to her 10-10 would be a valid id. I just tried it out and it looks Kube seems to be happy with it.
Looks like we only want to restrict 1234. Do you agree or have a counter argument?

If 10-10 is valid but 1234 is not, I wonder where the restriction is coming from. For what it's worth, 1234 is a valid identifier in k8s as well (you have to quote it in yaml otherwise it's interpreted as an integer).

The only checks for "not all numbers" I know of are

Places that use RFC1035 DNS labels (must start with an alphabetic character) -- this regex doesn't match this as 1a passes but is invalid.

Port names, for which 10-10 is invalid

In this case, what is the isInt check being used for specifically? I tried applying a DevWorkspace with ids like "1234" littered throughout and I didn't run into any issues -- the DevWorkspace operator correctly named a container 1234.

Actually I wasnt able to create a svc with 1234/12-34. Not sure what resource the DevWorkspace was creating 🤔. I even quoted it in yaml :

The Service "1234" is invalid: metadata.name: Invalid value: "1234": 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])?')

But anyhow, I think we can remove this restriction. I confirmed and it seems like port name cannot take either 1234/12-34 like you suggested and linked. And the library generator is already handling this for container port names from devfile.

Command IDs are not representative of any Kube resource and this just leaves us with Component name. I was able to create a pod with container named "1234"/"12-34".

WDYT?

Makes sense to me, it seems like isInt is unnecessary (I believe the 1234 svc name is covered by in-schema regexes)

Updated! I have also addressed the remaining unresolved conversations.

maysunfaisal · 2021-01-29T15:31:28Z

@amisevsk Any updates on the addressed commits? Thx!

amisevsk

Overall LGTM, a few leftover minor nits.

amisevsk · 2021-02-01T16:32:05Z

pkg/validation/commands.go

+)
+
+// ValidateCommands validates the devfile commands and checks:
+// 1. the command id is not all numeric


Doc is out of date since we removed the isInt requirement.

amisevsk · 2021-02-01T16:32:56Z

pkg/validation/commands.go

+// 1. the command id is not all numeric
+// 2. there are no duplicate command ids
+// 3. the command type is not invalid
+// 4. commands belonging to a specific group obeys the rule of 1 default command


Maybe clearer wording:

Suggested change

// 4. commands belonging to a specific group obeys the rule of 1 default command

// 4. if a command is part of a command group, there is a single default command

pkg/validation/commands.go

pkg/validation/components.go

pkg/validation/components_test.go

Signed-off-by: Maysun J Faisal <[email protected]>

Signed-off-by: Stephanie <[email protected]>

Signed-off-by: Maysun J Faisal <[email protected]>

Signed-off-by: Stephanie <[email protected]>

Signed-off-by: Maysun J Faisal <[email protected]>

Signed-off-by: Stephanie <[email protected]>

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal · 2021-02-01T21:24:21Z

@amisevsk thanks for those nit reviews! 🙌 I've addressed them!

amisevsk

Approving again to say LGTM! Thanks for your patience @maysunfaisal

openshift-ci-robot · 2021-02-02T16:46:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, maysunfaisal
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

maysunfaisal · 2021-02-02T17:27:49Z

@amisevsk no thank you for the thorough review! Appreciate it.

openshift-ci-robot added the do-not-merge/work-in-progress label Jan 13, 2021

maysunfaisal mentioned this pull request Jan 14, 2021

Odo should use apply command for preStart(and postStop when its implemented) redhat-developer/odo#4187

Closed

maysunfaisal force-pushed the 151-2 branch 2 times, most recently from 053441e to 692a917 Compare January 15, 2021 21:40

davidfestal requested a review from amisevsk January 18, 2021 13:40

maysunfaisal marked this pull request as ready for review January 18, 2021 15:20

maysunfaisal requested review from davidfestal and sleshchenko as code owners January 18, 2021 15:20

openshift-ci-robot removed the do-not-merge/work-in-progress label Jan 18, 2021

amisevsk reviewed Jan 18, 2021

View reviewed changes

maysunfaisal mentioned this pull request Jan 19, 2021

Use validations as specified by devfile library & api redhat-developer/odo#4361

Closed

johnmcollier reviewed Jan 20, 2021

View reviewed changes

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

maysunfaisal force-pushed the 151-2 branch from a30ce46 to 99a73ac Compare January 20, 2021 22:41

maysunfaisal requested a review from amisevsk January 20, 2021 22:41

amisevsk reviewed Jan 21, 2021

View reviewed changes

amisevsk approved these changes Feb 1, 2021

View reviewed changes

openshift-ci-robot assigned amisevsk Feb 1, 2021

openshift-ci-robot added the lgtm label Feb 1, 2021

maysunfaisal and others added 10 commits February 1, 2021 15:40

validation - odo parity

f3bd263

Signed-off-by: Maysun J Faisal <[email protected]>

add two util functions

ebf96cc

Signed-off-by: Stephanie <[email protected]>

Add command validations

fb1d03b

Signed-off-by: Maysun J Faisal <[email protected]>

Update event validation

a95579e

Signed-off-by: Maysun J Faisal <[email protected]>

add some unittests

4c12ed4

Signed-off-by: Stephanie <[email protected]>

validation - odo parity

b8965f3

Signed-off-by: Maysun J Faisal <[email protected]>

run go fmt

9238612

Signed-off-by: Stephanie <[email protected]>

Add project validations

19a7296

Signed-off-by: Maysun J Faisal <[email protected]>

Clean up PR

3bd2c8a

Signed-off-by: Maysun J Faisal <[email protected]>

Address PR reviews 1

d193765

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal added 4 commits February 1, 2021 15:40

Address PR reviews 2

ec29399

Signed-off-by: Maysun J Faisal <[email protected]>

Address PR review for tests

4a7f24e

Signed-off-by: Maysun J Faisal <[email protected]>

Update id and name validation

041949a

Signed-off-by: Maysun J Faisal <[email protected]>

Address PR reviews 3

f5954c4

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal force-pushed the 151-2 branch from 22c55f6 to f5954c4 Compare February 1, 2021 21:22

openshift-ci-robot removed the lgtm label Feb 1, 2021

amisevsk approved these changes Feb 2, 2021

View reviewed changes

openshift-ci-robot added the lgtm label Feb 2, 2021

maysunfaisal merged commit 6424f41 into devfile:master Feb 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add devfile validation #301

Add devfile validation #301

maysunfaisal commented Jan 13, 2021 •

edited

Loading

amisevsk left a comment

amisevsk Jan 21, 2021

amisevsk Jan 21, 2021

amisevsk Jan 21, 2021

amisevsk Jan 21, 2021

amisevsk Jan 21, 2021

amisevsk Jan 21, 2021

maysunfaisal Jan 25, 2021

amisevsk Jan 25, 2021 •

edited

Loading

maysunfaisal Jan 25, 2021

amisevsk Jan 26, 2021

maysunfaisal Jan 26, 2021

maysunfaisal commented Jan 29, 2021

amisevsk left a comment

amisevsk Feb 1, 2021

amisevsk Feb 1, 2021

maysunfaisal commented Feb 1, 2021

amisevsk left a comment

openshift-ci-robot commented Feb 2, 2021

maysunfaisal commented Feb 2, 2021

	// 4. commands belonging to a specific group obeys the rule of 1 default command
	// 4. if a command is part of a command group, there is a single default command

Add devfile validation #301

Add devfile validation #301

Conversation

maysunfaisal commented Jan 13, 2021 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is your PR tested? Consider putting some instruction how to test your changes

Docs PR

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amisevsk Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal commented Jan 29, 2021

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal commented Feb 1, 2021

amisevsk left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 2, 2021

maysunfaisal commented Feb 2, 2021

maysunfaisal commented Jan 13, 2021 •

edited

Loading

amisevsk Jan 25, 2021 •

edited

Loading