Parent Devfiles - Githubissues

l0rd commented 4 years ago

We want to experiment how the Devfile format could be used to define a stack.

Requirements

1. Parent devfile

A parent devfile is a devfile published somewhere where odo or Che can read it. Like a gist, a registry or a kubernetes resource (see referencing parents below).

A parent devfile is a regular devfile and includes components (for build and runtime), one or more code sample projects, events and commands to build and run the sample applications.

Any devfile that references a parent devfile will inherit its components, events and commands but won't inherit the code sample.

2. Referencing parents: uri, id or kubernetes

A parent devfile can be referenced in 3 different ways. Using its id if it has been published in a registry:

parent:
    id: redhat/nodejs/11.6  # <-- the id format is <publisher>/<stack-name>/<version>
    registry: https://devfile-registry.io/

Using the URI if it has been published on a static http server (like gist or pastebin):

parent:
    uri: https://raw.githubusercontent.com/eclipse/che-devfile-

Using a Kubernetes resource name, namespace if it has been deployed on a Kubernete cluster as a DevWorkspaceTemplate:

parent:
    kubernetes:
        name: mydevworkspacetemplate
        namespace: mynamespace

3. Customizing parent configuration

When referencing a parent, a devfile should be able to customize its configuration. The syntax to customize a parent configuration is similar to kustomize:

parent:
  id: redhat/theia-vsx-stack/latest     # <--- Parent referenced by id 
  components:                                  # <--- Parent configuration can be customized
    - name: vsx-installer                    # <--- Should match the name of an existing parent component
      container:
         env:
            - name: VSX_LIST
               value: java-dbg.vsix,java.vsix
  commands:
    (...)
components:                          # The main Devfile body provides new elements, not inherited from the parent
  - name: tooling                     # <--- Should *not* match the name of a parent component 
    container:                           # <--- Components are added to parent's components
      image: busybox
events:                                   # <--- Events are only allowed in the main Devfile body,
  (...)                                       #        and the resulting events is the merged of the these events with the events of the parent.

The place where an customizable element (component, command or project) should be defined is clearly driven by the following rule. It is either:

inside the scope of the parent element itself if it is an override of an existing parent element,
at the toplevel of the current devfile if it is a new element not present in the parent.

As a consequence;

If a command / project / component element is added inside the parent scope, but there is no element with the same key (name or id) in the parent, this will trigger an error.
If a command / project / component element is added in the main Devfle body, at the top-level, and an element with the same key (name or id) exists in the parent or in any plugin, then this will trigger an error.

As for the events element it is not really customizable: Due to the structure of the Events object, we can only add command bindings to some event. Overriding an existing command binding doesn't really makes sense. That's why we only allow events in the main devfile body, and not in the overrides. And the resulting events are the merge of the event objects coming from the parent, the plugins, and the main devfile body.

An Example

The parent devfile is published on github repo: https://raw.githubusercontent.com/eclipse/che-devfile-registry/master/devfiles/nodejs/devfile.yaml

The following devfile references it:

schemaVersion: 2.0.0
metadata:
  name: nodejs-app
parent:
    uri: https://(...)/nodejs/devfile.yaml # <--- Parent referenced by `uri`, registry `id`
                                           #      or `kubernetes` devworkspace
  components:                              # <--- Parent configuration can be customized
    - name: vsx-installer
      container:
         env:
            - name: VSX_LIST
               value: java-dbg.vsix,java.vsix
components:                               # <--- components are added to parent's components
  - name: tooling                       # <--- should not match the name of a parent component
    container:
      image: busybox
commands:                                 # <--- commands are added to parent's commands
   (...)

johnmcollier commented 4 years ago

@l0rd Could the project devfile also include additional components not referenced in the stack devfile (e.g. new containers, plugins, etc)?

l0rd commented 4 years ago

@l0rd Could the project devfile also include additional components not referenced in the stack devfile (e.g. new containers, plugins, etc)?

That's a good question. I think that we need to discuss that. There are 2 possible behaviors. Eventual components or commands in the project devfile:

replace the stack components or commands or
are added to the stack components or commands

The replace behavior is consistent with what we are doing with the project. But "augmenting" a stack with extra components looks like an interesting use case.

elsony commented 4 years ago

The goal is to allow user to refer to an existing devfile to reuse and modify/customerize it. Therefore, ideally, we should have a way to cover both adding definition and modifying existing one.

l0rd commented 4 years ago

Makes sense @elsony. We need to decide how we specify if a component/command is supposed to replace or extend a stack components/commands. Using IDs/aliases is an option.

davidfestal commented 4 years ago

I implemented the related changes, API-wise and spec-wise, in the proposal-25-variant-1-define-stacks.

I also added related examples of both:

a devfile 2.0.0 with devfile 2.0.0 parent
devWorkspace K8S CR with either DevWorkspaceTemplate CR parent or devfile 2.0.0 parent.

sbose78 commented 4 years ago

How do I try out the PoC ? :)

davidfestal commented 4 years ago

The only thing to try out on the Che side for now is playing with the syntax / API by running Openshift.io on this branch:

https://che.openshift.io/f/?url=https://github.com/che-incubator/devworkspace-api/tree/proposal-25-variant-1-define-stacks

As soon as the workspace is opened, you should be able to:

open the yaml files in the following folders:
- samples/
- devfile-support/samples
have yaml language support (completion and documentation) based on the current Json schemas.

I'm still working on the implementation side of this POC to include this parent mechanism into the existing Workspace CRD controller POC.

davidfestal commented 4 years ago

Here is a link to the demo of the POC implementation of this proposal on the Che side: https://youtu.be/SZppcZLQ03E

l0rd commented 4 years ago

The new details provided in the issue description have been reviewed and approved today

mik-dass commented 4 years ago

@davidfestal @l0rd @elsony If a devfile has a container defined with some env vars

parent:
  id: redhat/theia-vsx-stack/latest     # <--- Parent referenced by id 
  components:                           # <--- Parent configuration can be customized
    - container:
         name: vsx-installer
         env:
            - name: foo
               value: java-dbg.vsix,java.vsix

and the parent has a container defination with some extra env vars

 components:                          
    - container:
         name: vsx-installer
         env:
            - name: bar
               value: java.vsix

should these env vars be appended? e.g

 components:                          
    - container:
         name: vsx-installer
         env:
           - name: foo
               value: java-dbg.vsix,java.vsix
           - name: bar
               value: java.vsix

or should the parent's env vars be replaced?

l0rd commented 4 years ago

There was a discussion about that on gitter. Basically in this case we should use the Strategic Merge Patch and append the env variables.

kadel commented 4 years ago

If we are using Strategic Merge Path why we need the components projects events commnad sections inside parent?

This syntax is way too confusing. And it is not how Strategic Merge Path works in Kubernetes. I don't know why I didn't notice that before.

schemaVersion: 2.0.0
metadata:
  name: nodejs-app
parent:
    uri: https://(...)/nodejs/devfile.yaml # <--- Parent referenced by `uri`, registry `id`
                                           #      or `kubernetes` devworkspace
  components:                              # <--- Parent configuration can be customized
    - container:
         name: vsx-installer
         env:
            - name: VSX_LIST
               value: java-dbg.vsix,java.vsix
components:                               # <--- components are added to parent's components
  - container:
      name: tooling                       # <--- should not match the name of a parent component
      image: busybox
commands:                                 # <--- commands are added to parent's commands
   (...)

Something like the following example can do the same with a much clearer syntax. You simply lay the devfile on top of parent devfile, using Strategic Merge Patch logic, no need for extra fields in parent senction.

schemaVersion: 2.0.0
metadata:
  name: nodejs-app
parent:
    uri: https://(...)/nodejs/devfile.yaml # <--- Parent referenced by `uri`, registry `id`

components:                               
  - container:
      name: tooling                       
      image: busybox
   - container:
         name: vsx-installer
         env:
            - name: VSX_LIST
               value: java-dbg.vsix,java.vsix
commands:                                 
   (...)

if we want to provide more control over overriding or merging we can implement $path directive as described in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/strategic-merge-patch.md

davidfestal commented 4 years ago

@kadel There's still a difference, schema-wise, between the components projects events command sections inside parent compared to those at the root of the devfile:

The ones inside parent are overrides: that means that all the fields (recursively), apart from the top-level name, are optional. The overrides have no mandatory fields. On the contrary, root elements, which are complete elements of the current devfile, can have mandatory fields at various levels: for eample targetPort for container endpoints, commandLine for exec commands, image for container components.

It seems to me that mixing complete definition of devfile elements (root elements), with devfile partial fragments (overrides) is just mixing to things that are different in nature. In consequence it would force us to recursively remove any mandatory field from the Devfile schema.

If you take the kustomize use case, SMP patches (== Kustomize overlays == devfile overrides) are clearly distinguished from the base definition that should be overridden. Keeping the devfile overrides in a dedicated section (either the parent or the plugin scope) does the same, and allows distinguishing between plain definitions and overrides.

By the way, the current definition was designed to be consistent between parent and plugin elements: when you define a plugin, you add the overrides of the plugin elements inside the plugin definition itself. I don't really see why it should be different for parent.

Of course, in the current design, when you have root devfile elements, the root devfile elements should not exist (with the same name or id) in the parent or in any of the plugins. This is an error.

if we want to provide more control over overriding or merging we can implement $path directive as described in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/strategic-merge-patch.md

Sure, but isn't it another part of the subject, that would be implemented later on ? $patch directives are especially aimed at managing use-cases like deletion, that are not possible with basic fragment overlays.

kadel commented 4 years ago

To be honest, this is really confusing for users. It is not clear when I would specify something under parent or in the root of the file, users will be completely confused by this.

If the only reason to do this is to make validation of required fields easier than it is not a good tradeoff.

davidfestal commented 4 years ago

We have to take in account how plugins are overriden as well.

davidfestal commented 4 years ago

Also documentation is different in overrides than in root-level elements, for example:

Overrides of commands encapsulated in a parent devfile or a plugin. Overriding is done using a strategic merge patch

davidfestal commented 4 years ago

@kadel

Maybe it could make sense to only allow overriding existing elements in both parent and plugin scopes. And only allow adding new elements in the main devfile ?

Would it make it :

clearer for the parent case,
still consistent with the plugin case
Ensure that incomplete fragment definitions are only allowed when overriding an already existing complete definition (from either a parent or a plugin).

davidfestal commented 4 years ago

We discussed this during devfile 2.0 meeting, and agreed that the proposal in previous comment seems the way to go, and a good tradeof between consistency, schema-aware validation and clarity for the users.

It clearly defines the place where an element (component, command or project) can be added. Either:

in the parent element itself if it is an override of an existing parent element,
at the toplevel of the current devfile if it is a new element not present in the parent.

The following example summarizes it:

schemaVersion: 2.0.0
metadata:
  name: parent-and-plugin-overriding
  type: workspace

# First inherit from a Java technical stack as a parent
parent:
  id: redhat/java-stack/latest

  # Here we override *EXISTING* elements from the parent.
  # That's why it is *INSIDE* the `parent` element scope
  # If a new command / project / component element is added here that doesn't exist
  # in the parent, this will trigger an error  
  commands:
    # We're just overriding the `workingDir` field and MAVEN_OPTS env variable of the existing build command. 
    - exec:
        id: build
        workingDir: '${PROJECTS_ROOT}/spring-boot-http-booster'
        env:
          - name: MAVEN_OPTS
            value: "-Xmx400m"
  components:
    # We're just changing the memory limit and the volume size of the java plugin that exists in the parent. 
    - plugin:
        id: redhat/java8/latest
        components:
          - container:
              name: vscode-java
              memoryLimit: 2Gi
          - volume:
              name: m2
              size: 2G

# Now, in the main Devfile body, we add *NEW ELEMENTS*.
# If a command / project / component element with the same key (name or id) exists in the parent or in any plugin,
# then this will trigger an error  
projects:
  - name: spring-boot-http-booster
    git:
      location: https://github.com/snowdrop/spring-boot-http-booster
      branch: master
components:
  # New plugin that is not present in the parent stack
  # We can override an existing command of this plugin to change the working directory 
  - plugin:
      id: redhat/dependency-analytics/latest
      commands:
          - exec:
              id: analyze
              workingDir: '${PROJECTS_ROOT}/spring-boot-http-booster'
  # New container that is not present in the parent stack
  # and contains the developer personal tooling
  - container:
      name: my-personnal-tooling
      image: myrepo/mytooling:latest
      mountSources: true
commands:
  # New command that is not present in the parent stack
  # and refers to the new personnal tooling container
  - exec:
      id:my-own-command
      component: my-personnal-tooling
      workingDir: '${PROJECTS_ROOT}/spring-boot-http-booster'
      commandLine: ...

We didn't discuss the case of events, which is not a list. However, due to the structure of the Events object, we can only add command bindings to some event. Overriding an existing command binding doesn't really makes sense. So for now it would be consistent to only allow events in the main devfile body, and not in the overrides.

davidfestal commented 4 years ago

The strategic merge patch overriding mechanism has been implemented in PR #98 according to the agreement described in comment https://github.com/devfile/api/issues/25#issuecomment-668672087

elsony commented 4 years ago

@davidfestal @kadel It looks like the parent and plugin overriding and strategic merge patch have been implemented. Can this issue be closed off or is there anything still outstanding for this item?

davidfestal commented 4 years ago

@elsony Yes, it has been impmemented. So I assume we can close this issue.

However, if possible, I'd like to update the description first, according to the last agreement described in comment https://github.com/devfile/api/issues/25#issuecomment-668672087.

Also, do you think it would be necessary to agree on where is the reference implementation of this specified logic, before closing the issue ?

davidfestal commented 4 years ago

However, if possible, I'd like to update the description first, according to the last agreement described in comment #25 (comment).

Done

elsony commented 4 years ago

Confirmed the existing odo code is already using the strategic merge patch code as the api repo. Closing this issue.

devfile / api

Parent Devfiles #25

Requirements

An Example