Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command group (application lifecycle) #27

Closed
kadel opened this issue Feb 7, 2020 · 13 comments · Fixed by #35
Closed

Command group (application lifecycle) #27

kadel opened this issue Feb 7, 2020 · 13 comments · Fixed by #35
Milestone

Comments

@kadel
Copy link
Member

kadel commented Feb 7, 2020

Commands should have a group field that will specify in what point of application lifecycle the command should be executed (or type of the command like, build, incremental-build, run etc..).

  • group field is optional
  • Many commands of the same devfile can belong to the same group
  • Only one command belonging to a group can be the default
  • The list of groups should be: build, run, test, debug
commands:
 - exec:
     name: "package"
     commandLine: "mvn package"
     component: build-tools
     group: 
        kind: build
 - exec:
     name: "install"
     commandLine: "mvn install"
     component: build-tools
     group: 
         kind: build
         isDefault: true 
@kadel kadel changed the title Commands should have type (application lifecycle) Commands type (application lifecycle) Feb 7, 2020
@gorkem
Copy link

gorkem commented Feb 11, 2020

For reference vscode uses group instead of type.

@davidfestal
Copy link
Collaborator

I also prefer group instead of type, because it seems that type name is a bit confusing due to its frequent use in the context of sub-typing, or polymorphism.

Here we're more thinking of partitioning / grouping Exec commands into several categories, which is why I like the group (or category) field name better.

@kadel
Copy link
Member Author

kadel commented Feb 12, 2020

Here we're more thinking of partitioning / grouping Exec commands into several categories, which is why I like the group (or category) field name better.

Completely agree. group or category is far better than type which is already overloaded term.

@l0rd l0rd mentioned this issue Feb 18, 2020
28 tasks
@kadel kadel changed the title Commands type (application lifecycle) Command type/category (application lifecycle) Feb 18, 2020
@l0rd
Copy link
Contributor

l0rd commented Feb 25, 2020

We reviewed this proposal and agreed on the following specs:

  • We should use group instead of type for this proposal
  • group field is optional
  • Many commands of the same devfile can belong to the same group
  • Only one command belonging to a group can be the default

We started discussing whether we should have or not a default command for a given group like

commands:
 - exec:
     name: "package"
     commandLine: "mvn package"
     component: build-tools
     group: 
        name: build
 - exec:
     name: "install"
     commandLine: "mvn install"
     component: build-tools
     group: 
         name: build
         isDefault: true 

or

commands:
 - exec:
     name: "package"
     commandLine: "mvn package"
     component: build-tools
     group: build
 - exec:
     name: "install"
     commandLine: "mvn install"
     component: build-tools
     group: build
 - groupsDefaults:
     - group: "build"
       command: "install" 

or

commands:
 - exec:
     name: "package"
     commandLine: "mvn package"
     component: build-tools
     group: build
 - exec:
     name: "install"
     commandLine: "mvn install"
     component: build-tools
     group: build
 - groupsDefaults:
     - build: "install" 

@gorkem
Copy link

gorkem commented Feb 25, 2020

Is there a list of recognized 'group's?

@l0rd
Copy link
Contributor

l0rd commented Feb 25, 2020

@gorkem we haven't discussed that but haven't finished to discuss this issue. We will continue next week.

@davidfestal
Copy link
Collaborator

I assume that, in the second example proposed in comment #27 (comment), adding a single groupDefaults element at the end of the list of commands would not be very easy to define, both in Go and in the derived JSON schema, since it's a completely different schema, and not really a Command.
Additionally afaik enforcing it to appear only once through the schema validation might be impossible at this location.

commands:
 - exec:
     name: "package"
     commandLine: "mvn package"
     component: build-tools
     type: build
 - exec:
     name: "install"
     commandLine: "mvn install"
     component: build-tools
     type: build
 - groupsDefaults:
     - group: "build"
       command: "install" 

@l0rd l0rd changed the title Command type/category (application lifecycle) Command group (application lifecycle) Mar 3, 2020
@l0rd
Copy link
Contributor

l0rd commented Mar 3, 2020

Discussed and agreed

@groeges
Copy link

groeges commented Mar 17, 2020

Is there any way to define different commandLines dependant on which Operating System is being used, specifically for group: "init" (or group: "initLocal") which will be run on the users local system?

@l0rd
Copy link
Contributor

l0rd commented Mar 18, 2020

@groeges given that the commands are supposed to be executed in a container (components are k8s/openshift/container objects) and not as a local processes, supporting multiple OSes doesn't look like a valid use case.

@groeges
Copy link

groeges commented Mar 18, 2020

@l0rd I agree that most of the commands will be running within the container so a single script is OK, however I thought there was a devInitLocal being added (or something like that) which will run a command to initialise the project on the users local workspace and this is the case that may need to have multiple commandLines based on OS. If this is not being added then this is not an issue, but otherwise it is.

@l0rd
Copy link
Contributor

l0rd commented Mar 18, 2020

@groeges we are discussing that in #30 but I consider that devfiles describe container-based development workspaces and making it suitable for local dev (i.e. running local processes, accessing local files) is out of scope. But we can discuss that.

davidfestal added a commit that referenced this issue Apr 3, 2020
Signed-off-by: David Festal <dfestal@redhat.com>
davidfestal added a commit that referenced this issue Apr 22, 2020
* Implement agreement on issue #18
* Implement agreement on issue #17
* Implement agreement on issue #21
* Implement agreement on issue #22
* Implement agreement on issue #27
* Implement agreement on issue #9
* Implement agreement on issue #10
* Fix PR comments about issue #9 PR comment #35 (comment)
* Fix PR comments about issue #17 PR comment #35 (comment)
* Implement agreement on issue #14 and  #19
* Add the spring boot example
* Implement agreement on issue #32
* Fix last PR comments

Signed-off-by: David Festal <dfestal@redhat.com>

Co-Authored-By: Sergii Leshchenko <sleshche@redhat.com>
@l0rd l0rd added this to the 2.0.0 milestone Jun 16, 2020
@davidfestal davidfestal linked a pull request Jun 23, 2020 that will close this issue
@davidfestal
Copy link
Collaborator

Implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants