Validate exec subcommands in a composite (#3658)

* Validate exec subcommands in composite

* Add integration test

pkg/devfile/adapters/common/command.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/openshift/odo/pkg/devfile/parser/data"
 	"github.com/openshift/odo/pkg/devfile/parser/data/common"
+	"github.com/pkg/errors"
 	"k8s.io/klog"
 )
 
@@ -143,7 +144,7 @@ func validateCommand(data data.DevfileData, command common.DevfileCommand) (err
 	if command.Composite != nil {
 		parentCommands := make(map[string]string)
 		commandsMap := GetCommandsMap(data.GetCommands())
-		return validateCompositeCommand(command.Composite, parentCommands, commandsMap)
+		return validateCompositeCommand(data, command.Composite, parentCommands, commandsMap)
 	}
 
 	// component must be specified
@@ -173,7 +174,7 @@ func validateCommand(data data.DevfileData, command common.DevfileCommand) (err
 }
 
 // validateCompositeCommand checks that the specified composite command is valid
-func validateCompositeCommand(compositeCommand *common.Composite, parentCommands map[string]string, devfileCommands map[string]common.DevfileCommand) error {
+func validateCompositeCommand(data data.DevfileData, compositeCommand *common.Composite, parentCommands map[string]string, devfileCommands map[string]common.DevfileCommand) error {
 	if compositeCommand.Group != nil && compositeCommand.Group.Kind == common.RunCommandGroupType {
 		return fmt.Errorf("composite commands of run Kind are not supported currently")
 	}
@@ -200,12 +201,16 @@ func validateCompositeCommand(compositeCommand *common.Composite, parentCommands
 
 		if subCommand.Composite != nil {
 			// Recursively validate the composite subcommand
-			err := validateCompositeCommand(subCommand.Composite, parentCommands, devfileCommands)
+			err := validateCompositeCommand(data, subCommand.Composite, parentCommands, devfileCommands)
 			if err != nil {
 				// Don't wrap the error message here to make the error message more readable to the user
 				return err
 			}
-
+		} else {
+			err := validateCommand(data, subCommand)
+			if err != nil {
+				return errors.Wrapf(err, "the composite command %q references an invalid command %q", compositeCommand.Id, subCommand.GetID())
+			}
 		}
 	}
 	return nil

pkg/devfile/adapters/common/command_test.go

@@ -1868,6 +1868,33 @@ func TestValidateCompositeCommand(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "Case 6: Invalid composite command, points to invalid exec command",
+			compositeCommands: []common.Composite{
+				{
+					Id:       id[3],
+					Commands: []string{id[0], id[1]},
+					Group:    &versionsCommon.Group{Kind: buildGroup},
+				},
+			},
+			execCommands: []common.Exec{
+				{
+					Id:          id[0],
+					CommandLine: command[0],
+					Component:   component,
+					Group:       &common.Group{Kind: runGroup},
+					WorkingDir:  workDir[0],
+				},
+				{
+					Id:          id[1],
+					CommandLine: command[1],
+					Component:   "some-fake-component",
+					Group:       &common.Group{Kind: runGroup},
+					WorkingDir:  workDir[1],
+				},
+			},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		devObj := devfileParser.DevfileObj{
@@ -1882,7 +1909,7 @@ func TestValidateCompositeCommand(t *testing.T) {
 			commandsMap := GetCommandsMap(devObj.Data.GetCommands())
 			parentCommands := make(map[string]string)
 
-			err := validateCompositeCommand(cmd.Composite, parentCommands, commandsMap)
+			err := validateCompositeCommand(devObj.Data, cmd.Composite, parentCommands, commandsMap)
 			if !tt.wantErr == (err != nil) {
 				t.Errorf("TestValidateAction unexpected error: %v", err)
 				return

tests/examples/source/devfiles/nodejs/devfileCompositeInvalidComponent.yaml

@@ -0,0 +1,52 @@
+schemaVersion: 2.0.0
+metadata:
+  name: nodejs
+  version: 1.0.0
+projects:
+  - name: nodejs-starter
+    git:
+      location: "https://github.com/odo-devfiles/nodejs-ex.git"
+components:
+  - container:
+      name: runtime
+      image: registry.access.redhat.com/ubi8/nodejs-12:1-36
+      memoryLimit: 1024Mi
+      mountSources: true
+      endpoints:
+        - name: http-3000
+          targetPort: 3000
+commands:
+  - exec:
+      id: install
+      component: runtime
+      commandLine: npm install
+      workingDir: ${CHE_PROJECTS_ROOT}/nodejs-starter
+      group:
+        kind: build
+        isDefault: false
+  - exec:
+      id: mkdir
+      component: fakecomponent
+      commandLine: mkdir /projects/testfolder
+      workingDir: ${CHE_PROJECTS_ROOT}/nodejs-starter
+      group:
+        kind: build
+        isDefault: false
+  - exec:
+      id: run
+      component: runtime
+      commandLine: npm start
+      workingDir: ${CHE_PROJECTS_ROOT}/nodejs-starter
+      group:
+        kind: run
+        isDefault: true
+  - composite:
+         id: buildAndMkdir
+         label: Build and Mkdir
+         commands:
+           - mkdir
+           - install
+         parallel: false
+         group: 
+            kind: build
+            isDefault: true

tests/integration/devfile/cmd_devfile_push_test.go

@@ -241,6 +241,17 @@ var _ = Describe("odo devfile push command tests", func() {
 			Expect(output).To(ContainSubstring("cannot indirectly reference itself"))
 		})
 
+		It("should throw a validation error for composite command that has invalid exec subcommand", func() {
+			helper.CmdShouldPass("odo", "create", "nodejs", "--project", namespace, cmpName)
+
+			helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), context)
+			helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfileCompositeInvalidComponent.yaml"), filepath.Join(context, "devfile.yaml"))
+
+			// Verify odo push failed
+			output := helper.CmdShouldFail("odo", "push", "--context", context)
+			Expect(output).To(ContainSubstring("references an invalid command"))
+		})
+
 		It("checks that odo push works outside of the context directory", func() {
 			helper.Chdir(currentWorkingDirectory)