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

Proposal 25 - variant 1 - define stacks #29

Merged
merged 20 commits into from
Feb 26, 2020

Conversation

davidfestal
Copy link
Collaborator

What does this PR do?

This PR includes, into the DevWorkspace API / Devfile 2.0 schema, the required syntax that was agreed on in order to allow to define stacks by introducing the concept of parent.

Examples

  • To use a parent devfile in a devfile 2.0 and add a command:
name: with-parent
schemaVersion: 2.0.0
parent:
  uri: https://raw.githubusercontent.com/che-incubator/devworkspace-api/proposal-25-variant-1-define-stacks/devfile-support/samples/nodejs-stack.devfile.yaml
commands:
  - exec:
      label: Say Hello
      commandLine: echo "hello"
      component: nodejs
  • To use a parent devfile in a DevWorkspace Custom resource and add a command:
apiVersion: "workspaces.ecd.eclipse.org/v1alpha1"
kind: "DevWorkspace"
metadata:
  "name": "with-nodejs-devfile-parent"
spec:
  started: true
  template:
    parent:
      uri: https://raw.githubusercontent.com/che-incubator/devworkspace-api/proposal-25-variant-1-define-stacks/devfile-support/samples/nodejs-stack.devfile.yaml
    commands:
      - exec:
          label: Say Hello
          commandLine: echo "hello"
          component: nodejs
  • To use a DevWorkspaceTemplate Custom Resource as a parent in a DevWorkspace Custom resource and add a command:
apiVersion: "workspaces.ecd.eclipse.org/v1alpha1"
kind: "DevWorkspace"
metadata:
  "name": "with-nodejs-template-parent"
spec:
  started: true
  template:
    parent:
      kubernetes:
        name: nodejs-stack
        namespace: stacks
    commands:
      - exec:
          label: Say Hello
          commandLine: echo "hello"
          component: nodejs

What issues does this PR fix or reference?

Related syntax proposal is in issue #25

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

Yes, it has been tested in POCs on both the Odo side and the Che Workspace Operator side.

... and fix related elements elsewhere in the model to keep consistency

Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
to be consistent with the Che Workspace Operator model

Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
... that are not supported in CRD schemas for now.

Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

@davidfestal davidfestal requested a review from l0rd February 25, 2020 09:54
@davidfestal davidfestal self-assigned this Feb 25, 2020
@@ -101,7 +101,7 @@ type VscodeConfigurationCommandLocation struct {

type VscodeConfigurationCommand struct {
BaseCommand `json:",inline"`
Location VscodeConfigurationCommandLocation `json:",inline"`
VscodeConfigurationCommandLocation `json:",inline"`
Copy link
Member

Choose a reason for hiding this comment

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

I saw that there is convention in go related to abbreviatures, like userId must be userID. VSCode seems the right writing

Suggested change
VscodeConfigurationCommandLocation `json:",inline"`
VSCodeConfigurationCommandLocation `json:",inline"`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Afaik this general GO convention isn't very consistent with K8S APIs practices (and bube-builder requirements): in many cases, the GO Struct field name should be consistent (apart from the 1st letter) with the json / yaml field name specified in the related tag.

Maybe we'd rather disable this GO linting rule since it seems it will not be possible to apply it everywhere, at least in the K8S API packages

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW if we think it is something that should still be done (though I don't know how due to kube-builder constraints), I'd rather do it globally in a distinct PR, as a distinct issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I've seen

VSCodeConfigurationCommandLocation `json:vscodeConfigurationCommandLocation",inline"`

e.g. https://github.com/kubernetes/api/blob/b1350b9e3bc2ae69d6ea049ce1a637799a916e7d/auditregistration/v1alpha1/types.go#L175,

but I also don't know all the details and requirements.

Parent *Parent `json:"parent,omitempty"`

// Predefined, ready-to-use, workspace-related commands
Commands []Command `json:"commands,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what if the parent already contains command, component, project with such identifier(name, alias)...
Is it a valid configuration? If yes - then how we merge objects, do we merge them or just override?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We replace (override) them when they have the same key (component alias, command name, project name, etc ...), and add them if not.
It's a simple rule that has been agreed on in a first round of this.
Future iterations might come later with the ability to:

  • remove a top-level element of a parent (project, command, component),
  • even provide more complete json-patch directives.

However the goal is to make this simple and fully validated for the typical use-case tackled by this PR.

@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

1 similar comment
@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

1 similar comment
@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

1 similar comment
@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

1 similar comment
@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

1 similar comment
@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

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.

+1 with additional improvements in a future issue.

Signed-off-by: David Festal <[email protected]>
@che-osio-probot-app
Copy link

Open Developer Workspace:
Contribute

@davidfestal davidfestal merged commit e023a38 into master Feb 26, 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.

3 participants