Refactor validate pkg for generic and odo validation #4085

maysunfaisal · 2020-10-06T16:11:25Z

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

What type of PR is this?
/kind cleanup
/kind code-refactoring

What does does this PR do / why we need it:
We need this PR to refactor the validation pkg to odo specific validation and devfile generic validation, so that the generic validation can be exported out to the ~~devfile/parser~~ devfile/api repo

Which issue(s) this PR fixes:

#4073

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:
Run the unit tests in the validate pkg

openshift-ci-robot · 2020-10-06T16:11:42Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cdrage

Thanks for refactoring and moving the tests!

/approve

cdrage · 2020-10-06T19:33:28Z

pkg/devfile/parser/data/common/command_helper.go

@@ -59,6 +59,16 @@ func (dc DevfileCommand) GetExecWorkingDir() string {
 	return ""
 }

+// IsExec checks if the command is an exec command
+func (dc DevfileCommand) IsExec() bool {


New function means new tests :)

Despite this test being extremely small haha, we should add a test function for this!

@cdrage yes, I'll add the new test in ;)

could you remove the lgtm label? it looks like it got added?

maysunfaisal · 2020-10-06T20:00:26Z

@mik-dass @adisky could you pls review this PR? This PR refactors the validation pkg into odo specific and devfile specific logic; so that the devfile logic can be moved out to devfile/api repo in the future

yangcao77 · 2020-10-07T02:02:24Z

You might want to rebase due to merge of this pr: https://github.com/openshift/odo/pull/3959/files

pkg/devfile/parser/data/common/command_helper.go

adisky

@maysunfaisal If generic validation is extracted out for library, so the library is going to provide separate function to validate each resource? or it is only the first step for refactoring?

i think odo does not need to call each function isEventValid, ValidateCommands etc, it should call a single function generic.Validate() and then if everything is good from generic validation, odo starts own validation, I think both the validation should be decoupled more.

pkg/devfile/parser/data/common/command_helper_test.go

adisky · 2020-10-07T08:41:24Z

pkg/devfile/validate/commands.go

 	"github.com/openshift/odo/pkg/devfile/parser/data/common"
+	genericValidation "github.com/openshift/odo/pkg/devfile/validate/generic"


I think the alias also not needed, generic.ValidateCommands is pretty intuitive

updated!, pls help review!

I've also added a single call for generic validation like you suggested and it makes sense.

And yes, I'm refactoring the generic validation to a separate package because down the road devfile/api will provide validation for all devfile data acc to the schema. So separating out the generic validation will help in moving it out when it's required. And Odo will remain with its specific validation for its usage. The issue to track this is devfile/api#151

mik-dass · 2020-10-07T10:36:46Z

pkg/devfile/validate/commands.go

 		err = validateCommand(command, commandsMap, components)
 		if err != nil {
 			return err
 		}
 	}

-	return
+	err = genericValidation.ValidateCommands(commands, commandsMap, components)


we can return here

mik-dass · 2020-10-07T10:43:30Z

pkg/devfile/validate/generic/commands.go

+		return ValidateCompositeCommand(&command, parentCommands, devfileCommands, components)
+	}
+
+	err = ValidateExecCommand(command, components)


we can return here

mik-dass · 2020-10-07T10:48:01Z

pkg/devfile/validate/components.go

-	if len(invalidVolumeMounts) > 0 {
-		return &MissingVolumeMountError{volumeName: strings.Join(invalidVolumeMounts, ",")}
-	}
+	err := genericValidation.ValidateComponents(components)


we can return here

mik-dass · 2020-10-07T10:59:33Z

pkg/devfile/validate/generic/events.go

+		isEventPresent := false
+
+		if _, ok := commands[strings.ToLower(eventName)]; ok {
+			isEventPresent = true


I think we don't need this variable. We can simplify it like

if _, ok := commands[strings.ToLower(eventName)]; !ok { klog.V(2).Infof("%s type event %s does not map to a valid devfile command", eventType, eventName) eventErrorMsg[strings.ToLower(eventName)] = append(eventErrorMsg[strings.ToLower(eventName)], fmt.Sprintf("event %s does not map to a valid devfile command", eventName)) }

mik-dass · 2020-10-07T11:34:32Z

pkg/devfile/validate/events.go

 			if command.GetID() == strings.ToLower(eventName) {
-				isEventPresent = true
-
-				// Check if the devfile command is valid
+				// Check if the devfile command is a valid odo devfile command
 				err := validateCommand(command, commands, components)


Since the commands map contain all the commands in the devfile and they are all validated before this call, can't we just check if a eventName is in the commands map instead of validating each event again? What am I missing?

updated!, pls help review!

girishramnani · 2020-10-07T14:35:40Z

perfect 1000 lines added!!

yangcao77 · 2020-10-07T04:08:25Z

pkg/devfile/validate/commands.go

 		err = validateCommand(command, commandsMap, components)
 		if err != nil {
 			return err
 		}
 	}

-	return
+	err = genericValidation.ValidateCommands(commands, commandsMap, components)


return genericValidation.ValidateCommands(commands, commandsMap, components)

yangcao77 · 2020-10-07T04:36:53Z

pkg/devfile/validate/components.go

-	if len(invalidVolumeMounts) > 0 {
-		return &MissingVolumeMountError{volumeName: strings.Join(invalidVolumeMounts, ",")}
-	}
+	err := genericValidation.ValidateComponents(components)


return genericValidation.ValidateComponents(components)

pkg/devfile/validate/generic/commands.go

maysunfaisal · 2020-10-07T16:01:32Z

@girishramnani I had to clean up some test codes which were commented out, so that added to the lines. Now its approx -200.

maysunfaisal · 2020-10-07T22:43:01Z

/retest

codecov · 2020-10-08T02:48:18Z

Codecov Report

Merging #4085 into master will decrease coverage by 0.18%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master    #4085      +/-   ##
==========================================
- Coverage   43.17%   42.98%   -0.19%     
==========================================
  Files         147      150       +3     
  Lines       12561    12464      -97     
==========================================
- Hits         5423     5358      -65     
+ Misses       6562     6534      -28     
+ Partials      576      572       -4

Impacted Files	Coverage Δ
pkg/devfile/parser/data/common/command_helper.go	`77.27% <ø> (-3.50%)`	⬇️
pkg/devfile/validate/errors.go	`0.00% <0.00%> (-43.75%)`	⬇️
pkg/devfile/validate/generic/validate.go	`0.00% <0.00%> (ø)`
pkg/devfile/validate/validate.go	`23.80% <0.00%> (+2.07%)`	⬆️
pkg/devfile/validate/generic/errors.go	`14.28% <14.28%> (ø)`
pkg/devfile/validate/components.go	`80.00% <50.00%> (-13.11%)`	⬇️
pkg/devfile/validate/generic/events.go	`27.27% <55.55%> (ø)`
pkg/devfile/validate/commands.go	`50.00% <66.66%> (-45.46%)`	⬇️
pkg/devfile/validate/generic/commands.go	`91.11% <91.11%> (ø)`
pkg/devfile/adapters/common/utils.go	`94.23% <100.00%> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1245a5...92e09f6. Read the comment docs.

adisky

Mostly looks good to me, a nit here update the comment https://github.com/openshift/odo/blob/master/pkg/devfile/parse.go#L41

adisky · 2020-10-12T12:45:32Z

pkg/devfile/validate/generic/errors.go

+type ExecCommandMissingComponentError struct {
+	commandId string
+}
+
+func (e *ExecCommandMissingComponentError) Error() string {
+	return fmt.Sprintf("exec command %q must reference a component", e.commandId)
+}
+
+// ExecCommandMissingCommandLineError returns an error if the exec command does not have a command line
+type ExecCommandMissingCommandLineError struct {
+	commandId string
+}
+
+func (e *ExecCommandMissingCommandLineError) Error() string {
+	return fmt.Sprintf("exec command %q must have a command", e.commandId)
+}


should this all be covered in a single error like MissingInformationError?

This error refactoring could be done in separate PR, for now its ok.

I've already refactored, just need to push my changes

adisky · 2020-10-12T12:52:57Z

pkg/devfile/validate/generic/errors.go

+
+func (e *InvalidCommandError) Error() string {
+	return fmt.Sprintf("the command %q should be of type %s", e.commandId, e.commandType)
+}
+
+// ExecCommandInvalidContainerError returns an error if the exec command references an invalid container component
+type ExecCommandInvalidContainerError struct {
+	commandId string
+}
+
+func (e *ExecCommandInvalidContainerError) Error() string {
+	return fmt.Sprintf("the command %q does not map to a container component", e.commandId)
+}
+
+// CompositeDirectReferenceError returns an error if the composite command directly references itself
+type CompositeDirectReferenceError struct {
+	commandId string
+}
+
+func (e *CompositeDirectReferenceError) Error() string {
+	return fmt.Sprintf("the composite command %q cannot reference itself", e.commandId)
+}
+
+// CompositeIndirectReferenceError returns an error if the composite command indirectly references itself
+type CompositeIndirectReferenceError struct {
+	commandId string
+}
+
+func (e *CompositeIndirectReferenceError) Error() string {
+	return fmt.Sprintf("the composite command %q cannot indirectly reference itself", e.commandId)
+}
+


I think the errors could me refactored more here, like instead of having separate error for each validation fail it could be more common InvalidCommand MissingInformation

adisky · 2020-10-12T12:55:52Z

pkg/devfile/validate/validate.go

-
-		// Validate all the events
-		if err := validateEvents(events, commandsMap, components); err != nil {
+		if err := validateCommands(commandsMap); err != nil {


I think ValidateContainerName defined below should also be part of generic validation, it could be added inside component validation

There are a few reasons why I did not do it:

validation pkg is generally for validating logic directly related to the devfile schema and enforcing it

having said point 1, this func validates how the kube resource is to be created which is not really a devfile data validation, and it should not have been in validate pkg in the first place and is relying on adapter util funcs which further adds reason behind my point no 1.

Odo is the only container based tool. Workspace may need it but I dont see Console using it? 🤔
Lmk, what you think!

@maysunfaisal It is ok to not to add in library, just wanted an opinion essentially it is the validation of component name specified in the devfile, the container component would be translated to containers to all other tools, no? what console is translating devfile containers to if specified?

@adisky I've updated the pkg to validate the component names and made it more cleaner, pls have a look! Thx.

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

openshift-ci-robot · 2020-10-14T14:39:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdrage

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~~ [cdrage]

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

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

adisky · 2020-10-15T07:15:50Z

pkg/devfile/validate/generic/components.go

+	"github.com/openshift/odo/pkg/devfile/parser/data/common"
+	"github.com/openshift/odo/pkg/util"
+	"k8s.io/apimachinery/pkg/api/resource"
+)


we should not import odo packages in library, can we move this function in generic package ?util.ValidateK8sResourceName

"github.com/openshift/odo/pkg/util"

not sure about this one, anyways the constants are not related with adapters, it is a devfile spec so can be moved in types package.

"github.com/openshift/odo/pkg/devfile/adapters/common

for below one i understand, it will get updated when we use types from devfile/api

github.com/openshift/odo/pkg/devfile/parser/data/common

So devfile/parser already has a copy of the util func ValidateK8sResourceName https://github.com/devfile/parser/blob/master/pkg/util/util.go#L1044. I plan on using that when its exported out. It will just be changing the import. Also this func is used by other pkgs in odo..

For adaptersCommon "github.com/openshift/odo/pkg/devfile/adapters/common", we are just using constants. There will be new const when its moved to devfile/parser. Since this is still in odo, I dont want to maintain two separate const for the same thing.

Lmk, what you think @adisky

adisky · 2020-10-15T14:28:11Z

/lgtm

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/cleanup labels Oct 6, 2020

openshift-ci-robot requested review from girishramnani and prietyc123 October 6, 2020 16:11

maysunfaisal marked this pull request as ready for review October 6, 2020 19:00

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Oct 6, 2020

cdrage approved these changes Oct 6, 2020

View reviewed changes

openshift-ci-robot assigned cdrage Oct 6, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. Required by Prow. approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. labels Oct 6, 2020

elsony removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Oct 6, 2020

yangcao77 reviewed Oct 7, 2020

View reviewed changes

pkg/devfile/parser/data/common/command_helper.go Outdated Show resolved Hide resolved

adisky reviewed Oct 7, 2020

View reviewed changes

mik-dass reviewed Oct 7, 2020

View reviewed changes

yangcao77 reviewed Oct 7, 2020

View reviewed changes

maysunfaisal force-pushed the 4073-1 branch from 8310d84 to 2d8cdde Compare October 7, 2020 22:41

adisky reviewed Oct 12, 2020

View reviewed changes

First pass of refactoring validate pkg for generic validation

20ab5ab

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

maysunfaisal added 5 commits October 13, 2020 10:58

Update tests and cleanup refactored code

8e491cd

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

Add test, clean up func desc

35a782e

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

Apply PR Feedback, cleanup code and tests

a48e288

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

Fix integratino test

684a95f

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

Refactor and cleanup validation

0c887e0

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

maysunfaisal force-pushed the 4073-1 branch from 0b60b21 to 0c887e0 Compare October 14, 2020 14:39

maysunfaisal added 2 commits October 14, 2020 11:25

Add component name validation to generic validation

8d4409c

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

Add one more test case for env

97c2eb5

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

maysunfaisal requested a review from adisky October 14, 2020 15:33

Update integration tests to be more generic

92e09f6

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

adisky reviewed Oct 15, 2020

View reviewed changes

openshift-ci-robot assigned adisky Oct 15, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Oct 15, 2020

openshift-merge-robot merged commit 84512b9 into redhat-developer:master Oct 15, 2020

yangcao77 mentioned this pull request Feb 2, 2021

Integrate devfile/api validator in devfile/library devfile/api#337

Closed

rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 16, 2023

		"github.com/openshift/odo/pkg/devfile/parser/data/common"
		genericValidation "github.com/openshift/odo/pkg/devfile/validate/generic"

Refactor validate pkg for generic and odo validation #4085

Refactor validate pkg for generic and odo validation #4085

Conversation

maysunfaisal commented Oct 6, 2020 • edited Loading

openshift-ci-robot commented Oct 6, 2020

cdrage 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 Oct 6, 2020

yangcao77 commented Oct 7, 2020

adisky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Oct 7, 2020 • 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

mik-dass Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

mik-dass Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girishramnani commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal commented Oct 7, 2020 • edited Loading

maysunfaisal commented Oct 7, 2020

codecov bot commented Oct 8, 2020 • edited Loading

Codecov Report

adisky 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

adisky Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

adisky Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

maysunfaisal Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 14, 2020

adisky Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

maysunfaisal Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisky commented Oct 15, 2020

maysunfaisal commented Oct 6, 2020 •

edited

Loading

maysunfaisal Oct 7, 2020 •

edited

Loading

mik-dass Oct 7, 2020 •

edited

Loading

mik-dass Oct 7, 2020 •

edited

Loading

maysunfaisal commented Oct 7, 2020 •

edited

Loading

codecov bot commented Oct 8, 2020 •

edited

Loading

adisky Oct 12, 2020 •

edited

Loading

adisky Oct 12, 2020 •

edited

Loading

maysunfaisal Oct 13, 2020 •

edited

Loading

adisky Oct 15, 2020 •

edited

Loading

maysunfaisal Oct 15, 2020 •

edited

Loading