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

Implementation of parent and plugin overriding #98

Merged
merged 24 commits into from
Aug 24, 2020

Conversation

davidfestal
Copy link
Collaborator

@davidfestal davidfestal commented Jul 17, 2020

What does this PR do?

This PR implements the parent or plugin overriding.

The overriding of parent devfile or devfile plugins is implemented using the Strategic merge patch K8S library.

Implementation entry points;

The implementation is available for import in the pkg/utils/overriding package.

Available methods are:

In both case the result is a DevWorkspaceTemplateSpecContent Go struct that corresponds to the code content of a devfile 2.0 (without the top-level apiVersion and metadata fields).

Note that one can only override an existing element of a parent or a plugin.

If a new element is added, it should be added in the main (child) devfile body. And then simple merging of top-level lists is done between the main devfile body, the flattened parent and the flattened plugins. If the key (name of ID) of a element is found several times in the merge, an error is thrown.

The corresponding methods are:

In both case the result is a DevWorkspaceTemplateSpecContent Go struct that corresponds to the code content of a devfile 2.0 (without the top-level apiVersion and metadata fields).

Examples of use:

YAML examples:

YAML example files are provided and used in GO tests.

Overriding

Overriding examples can be found in the following test-fixtures/patches folder: each sub-folder contains 3 Yaml files:

  • original.yaml: the original Yaml that should be overridden
  • patch.yaml: the Yaml patch that should be used to override the original file
  • result.yaml: the expected result of the overriding, which is the result of applying strategic merge patch on the original file.
Merging

Merging examples can be found in the following test-fixtures/merges folder: each sub-folder contains up to 4 Yaml files:

  • main.yaml: the Yaml of the main (child) devfile body content
  • parent.yaml: the Yaml of a flattened parent referenced by the main devfile
  • plugin.yaml: the Yaml a flattened plugin referenced by the main devfile
  • result.yaml: the expected result of the merging of all those devfile contents.

When opened in VSCode or Hosted Che, all these example files are validated with appropriate and up-to-date schemas. So they benefit from YAML language services such as completion and doc hovers.

Contributions are welcomed in the future to further complete the tests (and at the same time documentation) of overriding rules.

Noteworthy (and possibly breaking) changes

  • Events:
    • Detailed change: Instead of directly referencing the Events structure, the DevWorkspaceTemplateSpecContent struct contains a pointer to an Events struct.
    • Reason: This make testing easier, since it avoids a number of corner cases around serialization, when an empty Events object is created although no events field is present in the parsed json or yaml.
    • Impact: it will require a change in depending code, and a nil-check after json or yaml parsing.

Implementation details

Keys

Currently in the existing model, for some top-level lists (components, commands), the keys of elements are not directly in the objects stored in the list, but one level lower:

commands:
  - exec:
      id: "commandId"  <------ the key of the given command in the list of commands 

instead of

commands:
  - id: "commandId"
    exec:
      ....

Furthermore for some components as plugins, the key of a plugin in the list of components may be calculated, and may depend on a number of fields (id and registryUrl for example).
Thus a Keyed interface is defined and implemented for commands, components and component overrides, and it is then used to setup fixed keys in the commands and components lists of both the original object and the patch object during overriding.

Union normalizing

In the GO API, some model types are defined as Kubernetes unions with discriminators.

The API Union interface defines helpers to manage correctly manage those unions, especially maintaining the fact that values in a union are mutually-exclusive.

This union management logic is then used in the overriding implementation, but is also available in a dedicated pkg/utils/unions package that allows managing all the unions embedded in a wider object.

What issues does this PR fix or reference?

This PR references issue #91

Is your PR tested? Consider putting some instruction how to test your changes

The overriding logic implemented in this PR has associated tests

@davidfestal davidfestal requested a review from l0rd July 17, 2020 15:33
@davidfestal davidfestal changed the title WIP Implementation of parent and plugin overriding Implementation of parent and plugin overriding Jul 20, 2020
@davidfestal davidfestal requested a review from kadel July 20, 2020 14:22
@davidfestal davidfestal linked an issue Jul 20, 2020 that may be closed by this pull request
@kadel
Copy link
Member

kadel commented Jul 21, 2020

Going over this PR I noticed something that for some reason I didn't notice before.

The current design looks quite weird, and it is not how Strategic Merge Patch works in Kubernetes.

More in my comment at #25 (comment)

@davidfestal davidfestal force-pushed the overriding-implementation branch from d951c43 to 42182e5 Compare August 13, 2020 16:57
@davidfestal
Copy link
Collaborator Author

Going over this PR I noticed something that for some reason I didn't notice before.

The current design looks quite weird, and it is not how Strategic Merge Patch works in Kubernetes.

More in my comment at #25 (comment)

@kadel I implemented the agreement we found (summarized here ) in the PR, and updated the PR description to add details about the merging side of things (addition of elements that are not present in parents or plugins).

@davidfestal
Copy link
Collaborator Author

cc @ranakan19

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, with a few minor comments/nitpicks. I'll add a commit to resolve those for simplicity.

davidfestal and others added 17 commits August 20, 2020 00:43
... in the future these definitions could be generated automatically
from the api source code.

Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
The test yaml files provide useful hints abot how the overriding
works according to strategic merge patch rules

Signed-off-by: David Festal <[email protected]>
... and enhance markdown support to support
code blocks.

Signed-off-by: David Festal <[email protected]>
`targetPort` in `Kubernetes`-like components

Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
... And add new elements only in the main devfile body.

Signed-off-by: David Festal <[email protected]>
@amisevsk amisevsk force-pushed the overriding-implementation branch from 00b1c64 to 13fbbc9 Compare August 20, 2020 04:43
@amisevsk
Copy link
Contributor

I had to rebase on master as there was a conflict. Commit 13fbbc9 resolves the minor comments from my review.

@davidfestal
Copy link
Collaborator Author

Thanks @amisevsk for this deep review and very-valuable fixes.

@davidfestal davidfestal merged commit 4353bcc into master Aug 24, 2020
@davidfestal davidfestal deleted the overriding-implementation branch August 24, 2020 08:21
This was referenced Aug 24, 2020
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 this pull request may close these issues.

Overriding rules for parents and plugins
3 participants