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

Parent Devfiles #25

Closed
l0rd opened this issue Feb 6, 2020 · 23 comments
Closed

Parent Devfiles #25

l0rd opened this issue Feb 6, 2020 · 23 comments
Labels
area/api Enhancement or issue related to the api/devfile specification kind/enhancement New feature or request
Milestone

Comments

@l0rd
Copy link
Contributor

l0rd commented Feb 6, 2020

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
   (...)
@l0rd l0rd mentioned this issue Feb 6, 2020
28 tasks
@johnmcollier
Copy link
Member

johnmcollier commented Feb 6, 2020

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

@l0rd
Copy link
Contributor Author

l0rd commented Feb 6, 2020

@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
Copy link
Contributor

elsony commented Feb 6, 2020

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
Copy link
Contributor Author

l0rd commented Feb 6, 2020

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
Copy link
Collaborator

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:

@sbose78
Copy link

sbose78 commented Feb 13, 2020

How do I try out the PoC ? :)

@davidfestal
Copy link
Collaborator

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
Copy link
Collaborator

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

@l0rd
Copy link
Contributor Author

l0rd commented Apr 21, 2020

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

@l0rd l0rd changed the title Using the Devfile format to define Stacks [PoC] Parent Devfiles May 6, 2020
@l0rd l0rd added this to the 2.0.0 milestone Jun 16, 2020
@mik-dass
Copy link

@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
Copy link
Contributor Author

l0rd commented Jun 23, 2020

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

@l0rd l0rd added area/api Enhancement or issue related to the api/devfile specification kind/enhancement New feature or request labels Jun 24, 2020
@kadel
Copy link
Member

kadel commented Jul 21, 2020

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
Copy link
Collaborator

@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
Copy link
Member

kadel commented Aug 4, 2020

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
Copy link
Collaborator

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

@davidfestal
Copy link
Collaborator

davidfestal commented Aug 4, 2020

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
Copy link
Collaborator

@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
Copy link
Collaborator

davidfestal commented Aug 4, 2020

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
Copy link
Collaborator

The strategic merge patch overriding mechanism has been implemented in PR #98 according to the agreement described in comment #25 (comment)

davidfestal referenced this issue Aug 31, 2020
* Prepare GO model for overriding
* Keyed interface and union normalizing implementation
* Gather all union definitions in a single file. In the future these definitions could be generated automatically
from the api source code.
* Basic union normalizing tests
* Implement overriding logic
* Add overriding tests. The test yaml files provide useful hints about how the overriding
works according to strategic merge patch rules
* Add a schema for overrides and enhance markdown support to support code blocks.
* Add missing required fields: `targetPort` in `Kubernetes`-like components
* Generate files (crds,go and schemas)
* Add schemas for yaml files of overriding tests
* Add directives
* Add additional vendors
* Add a command to run tests
* Support the case when no `.theia` folder exists
* Only override in parent or plugin scope and add new elements only in the main devfile body.
* Add the merge of events.
* Remove the Events from the Overrides struct
* Generate CRDs and schemas
* Accommodate repo renaming

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

* Run go fmt on changes
* Run go mod tidy
* Minor code cleanups and formatting
* Regenerate schemas after rebasing to fix PR check

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Co-authored-by: David Festal <dfestal@dfestal.remote.csb>
Co-authored-by: Angel Misevski <amisevsk@redhat.com>
@elsony
Copy link
Contributor

elsony commented Oct 6, 2020

@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
Copy link
Collaborator

@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 #25 (comment).

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
Copy link
Collaborator

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

Done

@elsony
Copy link
Contributor

elsony commented Oct 20, 2020

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

@elsony elsony closed this as completed Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Enhancement or issue related to the api/devfile specification kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants