Migrate devfile cmd validation to validate pkg by maysunfaisal · Pull Request #3912 · redhat-developer/odo

maysunfaisal · 2020-09-08T21:15:51Z

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

What does does this PR do / why we need it:

migrates devfile command validation to validate pkg along with other devfile validation
devfile commands are now validated anytime we parse devfile and not only during odo push when using the command
cleaned up some code
updated unit tests

Which issue(s) this PR fixes:

Fixes #3703

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

Make invalid changes to the devfile command and any odo command that parses the devfile should err out

openshift-ci-robot · 2020-09-08T21:16:07Z

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

codecov · 2020-09-08T21:36:08Z

Codecov Report

Merging #3912 into master will decrease coverage by 0.02%.
The diff coverage is 73.40%.

@@            Coverage Diff             @@
##           master    #3912      +/-   ##
==========================================
- Coverage   44.55%   44.53%   -0.03%     
==========================================
  Files         143      145       +2     
  Lines       13979    14005      +26     
==========================================
+ Hits         6229     6237       +8     
- Misses       7158     7176      +18     
  Partials      592      592

Impacted Files	Coverage Δ
pkg/devfile/parser/data/2.0.0/components.go	`38.37% <ø> (ø)`
pkg/devfile/validate/validate.go	`0.00% <0.00%> (ø)`
pkg/devfile/validate/errors.go	`40.00% <33.33%> (-10.00%)`	⬇️
pkg/devfile/validate/events.go	`85.71% <71.42%> (-0.57%)`	⬇️
pkg/devfile/validate/commands.go	`95.55% <95.55%> (ø)`
pkg/devfile/adapters/common/command.go	`95.48% <100.00%> (+0.08%)`	⬆️
pkg/devfile/adapters/common/utils.go	`94.36% <100.00%> (-0.70%)`	⬇️
pkg/devfile/parser/data/common/component_helper.go	`100.00% <100.00%> (ø)`

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 0730745...7bc997d. Read the comment docs.

mik-dass · 2020-09-11T08:08:46Z

pkg/devfile/adapters/common/utils_test.go

+// func TestIsContainer(t *testing.T) {
+
+// 	tests := []struct {
+// 		name            string
+// 		component       common.DevfileComponent
+// 		wantIsSupported bool
+// 	}{
+// 		{
+// 			name:            "Case 1: Container component",
+// 			component:       testingutil.GetFakeContainerComponent("comp1"),
+// 			wantIsSupported: true,
+// 		},
+// 		{


these comments need to be removed

mik-dass · 2020-09-11T08:09:55Z

pkg/devfile/parse.go


-	// Successful
-	return d, nil
+	return d, err


I think these changes can be applied in the above functions as well

mik-dass · 2020-09-11T08:10:42Z

pkg/devfile/validate/commands.go

+// validateCommands validates all the devfile commands
+func validateCommands(commands map[string]common.DevfileCommand, components []common.DevfileComponent) (err error) {
+
+	// commandsMap := adapterCommon.GetCommandsMap(commands)


I think this can be removed

mik-dass · 2020-09-11T08:11:41Z

pkg/devfile/validate/commands.go

+	// If the command is a composite command, need to validate that it is valid
+	if command.Composite != nil {
+		parentCommands := make(map[string]string)
+		// commandsMap := adapterCommon.GetCommandsMap(commands)


I think this comment can be removed

mik-dass · 2020-09-11T08:23:35Z

Tests seem to be failing on Travis
https://travis-ci.com/github/openshift/odo/jobs/383364891#L2065

maysunfaisal · 2020-09-11T20:15:52Z

/retest

mik-dass · 2020-09-15T07:23:03Z

CI seems to be failing

e "runtime"
         ✗  the command "mkdir" does not map to a container component
        
    to contain substring
        <string>: references an invalid command

maysunfaisal · 2020-09-15T13:03:09Z

CI seems to be failing

e "runtime"
         ✗  the command "mkdir" does not map to a container component
        
    to contain substring
        <string>: references an invalid command

@mik-dass yep, I noticed this yesterday

maysunfaisal · 2020-09-15T21:21:00Z

/retest

mik-dass

/lgtm

girishramnani · 2020-09-16T11:33:56Z

/approve

openshift-ci-robot · 2020-09-16T11:34:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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/devfile/OWNERS~~ [girishramnani]
~~tests/OWNERS~~ [girishramnani]

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 <maysunaneek@gmail.com>

johnmcollier

Readding the lgtm label due to rebase

/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 Sep 8, 2020

openshift-ci-robot requested review from mik-dass and mohammedzee1000 September 8, 2020 21:16

maysunfaisal mentioned this pull request Sep 9, 2020

Updates name and id field in the devfile schema #3872

Merged

4 tasks

maysunfaisal force-pushed the 3703-1 branch from 4bb320f to e3250ad Compare September 9, 2020 21:44

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 9, 2020

maysunfaisal force-pushed the 3703-1 branch from e3250ad to 1ec61ca Compare September 10, 2020 20:12

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 10, 2020

maysunfaisal marked this pull request as ready for review September 10, 2020 21:10

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 Sep 10, 2020

mik-dass reviewed Sep 11, 2020

View reviewed changes

maysunfaisal force-pushed the 3703-1 branch from 1ec61ca to 4e6bee1 Compare September 11, 2020 16:49

maysunfaisal force-pushed the 3703-1 branch from 4e6bee1 to f62df5c Compare September 14, 2020 17:14

maysunfaisal force-pushed the 3703-1 branch from f62df5c to 6a5c191 Compare September 15, 2020 18:16

mik-dass approved these changes Sep 16, 2020

View reviewed changes

openshift-ci-robot assigned mik-dass Sep 16, 2020

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 16, 2020

johnmcollier mentioned this pull request Sep 16, 2020

'odo create' should properly validate devfiles #3778

Closed

maysunfaisal added 7 commits September 16, 2020 11:33

first commit of command validation

d10080a

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

second commit of command validation

f368987

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Update validation errors

b54070b

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Cleanup code, move command validation

059cae2

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Update and fix Unit tests

9bceae4

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Update integration tests

ebacfba

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Dont ignore the validation err in composite cmd

7bc997d

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

maysunfaisal force-pushed the 3703-1 branch from 6a5c191 to 7bc997d Compare September 16, 2020 17:07

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 16, 2020

johnmcollier approved these changes Sep 16, 2020

View reviewed changes

openshift-ci-robot assigned johnmcollier Sep 16, 2020

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

openshift-merge-robot merged commit efb0448 into redhat-developer:master Sep 17, 2020

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

Conversation

maysunfaisal commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 8, 2020

Uh oh!

codecov bot commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mik-dass Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

mik-dass Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

mik-dass Sep 11, 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!

mik-dass Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

mik-dass commented Sep 11, 2020

Uh oh!

maysunfaisal commented Sep 11, 2020

Uh oh!

mik-dass commented Sep 15, 2020

Uh oh!

maysunfaisal commented Sep 15, 2020

Uh oh!

maysunfaisal commented Sep 15, 2020

Uh oh!

mik-dass left a comment

Choose a reason for hiding this comment

Uh oh!

girishramnani commented Sep 16, 2020

Uh oh!

openshift-ci-robot commented Sep 16, 2020

Uh oh!

johnmcollier left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maysunfaisal commented Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 8, 2020 •

edited

Loading

mik-dass Sep 11, 2020 •

edited

Loading