Exec command should make component required

[maysunfaisal]

Currently the spec does not define the component in an exec as required. I am wondering if that's the case, what is the point in have an exec command at all? If the there is an exec command in a devfile, it is to be executed in a container; which is to be represented by the component property.

https://github.com/devfile/api/blob/master/pkg/apis/workspaces/v1alpha2/commands.go#L125

[davidfestal]

@l0rd wdyt about this one ? It seems we should make the component mandatory right ?

[elsony]

I don't see much usage on a command without a component. Without the component info, the tools using the devfile won't know where are they going to execute the command.

[l0rd]

Agreed. The component should be mandatory for exec commands. On the other hand apply commands should not require that.

[mmulholla]

So this:

// Describes component to which given action relates
// +optional
Component string `json:"component,omitempty"`

Becomes:

// Describes component to which given action relates
// 
Component string `json:"component"`

?????

[davidfestal]

Yes, exactly.

[johnmcollier]

So, I think this is a good idea. But one thing I'm wondering is, will this cause issues for overriding parent commands in devfiles?

E.g.

schemaVersion: 2.0.0
metadata:
  name: nodejs
  version: 1.0.0
parent:
  uri: https://raw.githubusercontent.com/odo-devfiles/registry/master/devfiles/nodejs/devfile.yaml
  commands:
    - id: install
      exec:
        commandLine: npm install && echo hi

Is currently valid, and changes the commandLine for the install command. We don't specify the component of the overridden command since we're not changing it. Will this still be valid if we make component required?

[mmulholla]

For example, https://github.com/devfile/api/blob/master/samples/devfiles/simple-devfile.yaml#L30 includes:

• id: helloWorld
  exec:
  env:
  - name: "USER"
  value: "John Doe"
  commandLine: 'echo "Hello ${USER}"'

This example would now be invalid. Is this acceptable?

[maysunfaisal]

@mmulholla yes, that would become invalid because the exec command has no component to execute itself.

[mmulholla]

So given this and what John pointed out do we still want this change?

[davidfestal]

For example, https://github.com/devfile/api/blob/master/samples/devfiles/simple-devfile.yaml#L30 includes:

• id: helloWorld
  exec:
  env:
  • name: "USER"
    value: "John Doe"
    commandLine: 'echo "Hello ${USER}"'

This example would now be invalid. Is this acceptable?

@mmulholla @maysunfaisal

Yes, it seems this is an error in this example file, that was not reported because of the fact that component is optional for now.
IMO this should not prevent us from doing the change proposed in this issue.

[davidfestal]

So, I think this is a good idea. But one thing I'm wondering is, will this cause issues for overriding parent commands in devfiles?

E.g.

schemaVersion: 2.0.0
metadata:
  name: nodejs
  version: 1.0.0
parent:
  uri: https://raw.githubusercontent.com/odo-devfiles/registry/master/devfiles/nodejs/devfile.yaml
  commands:
    - id: install
      exec:
        commandLine: npm install && echo hi

Is currently valid, and changes the commandLine for the install command. We don't specify the component of the overridden command since we're not changing it. Will this still be valid if we make component required?

In fact that won't be a problem since the override generator takes care of that when generating the Override Structs. It makes all the fields optional, apart from those that are strictly required because they are used as merge keys in a list.

So even with the change proposed here to implement this issue, the command override described by @johnmcollier and quoted above will still be valid.

[davidfestal]

Agreed. The component should be mandatory for exec commands. On the other hand apply commands should not require that.

@l0rd Do you mean that Apply command should not require a mandatory component ? What would an Apply command without a component field mean ?

[l0rd]

Agreed. The component should be mandatory for exec commands. On the other hand apply commands should not require that.

@l0rd Do you mean that Apply command should not require a mandatory component ? What would an Apply command without a component field mean ?

@davidfestal you are right. It should be mandatory for apply commands as well.